Closed karlkfi closed 2 years ago
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: karlkfi
To complete the pull request process, please assign seans3 after the PR has been reviewed.
You can assign the PR to them by writing /assign @seans3
in a comment when ready.
The full list of commands accepted by this bot can be found here.
@karlkfi: The following tests failed, say /retest
to rerun all failed tests or /retest-required
to rerun all mandatory failed tests:
Test name | Commit | Details | Required | Rerun command |
---|---|---|---|---|
cli-utils-presubmit-master | f35447368d31ffad037fd955884fbb7f568d9f3b | link | true | /test cli-utils-presubmit-master |
cli-utils-presubmit-master-e2e | f35447368d31ffad037fd955884fbb7f568d9f3b | link | true | /test cli-utils-presubmit-master-e2e |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
I think I'm going to put this optimization on hold for now. It has both positive and negative performance impacts.
Pros:
Cons:
I'm not sure the pros outweigh the cons.
I think we can apply this optimization only to the objects that are immediately ready.
I think we can apply this optimization only to the objects that are immediately ready.
We would have to have a list of status readers specific to those "always ready" objects. Right now they're buried in the StatusPoller's defaultStatusReader. I can't think of a good way to expose them without duplicating the logic in two places. Plus, the StatusReader interface requires providing a ClusterReader, which we don't want it to use. So it'd be a big hack to try to share code, even if we wanted to.
Can you think of a better way to implement it that doesn't add a bunch of tech debt?
This change adds an immediate status check after apply using the response object. This should short-circuit waiting for the StatusPoller for objects that reconcile immediately (e.g. ConfigMaps).
Unfortunately, this won't help CRDs or Namespaces, which actually need their controller to update their status.
This PoC impl is a hack. To do ti right, we'll need to refactor the StatusPoller to expose the injected StatusReaders, otherwise the ones used after apply won't match the ones injected by the user. Alternatively, we could just have users inject a list of StatusReaders and construct the StatusPoller ourselves.