Allow GeneratingHandlers to skip Apply if resource version didn't change #341
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KevinJoiner
suggested changes
Dec 5, 2023
There was a problem hiding this comment.
Overall, this looks good. I don't love adding more code in generated templates, but this is a good approach to solving the problem.
Also, I didn't mark this but I would be forever grateful if you added function comments to GeneratingHandler since you are already in the package.
Co-authored-by: Kevin Joiner <[email protected]>
Co-authored-by: Kevin Joiner <[email protected]>
I'm sorry I missed this comment when I first addressed your feedback. It's done now, please take another look, thanks! |
KevinJoiner
approved these changes
Dec 19, 2023
aruiz14
added a commit
to aruiz14/wrangler
that referenced
this pull request
Jan 3, 2024
…nge (rancher#341) * Allow GeneratingHandlers to skip Apply if resource version didn't change * Run go generate * Add call count to FakeApply * Add test for UniqueApplyForResourceVersion * Simplify error handling Co-authored-by: Kevin Joiner <[email protected]> * Make generated code more readable Co-authored-by: Kevin Joiner <[email protected]> * Rename constant * Rename seenResourceVersion to storeResourceVersion * go generate * Add comments to code generator * Run go generate * Upgrade golangci-lint GitHub action, since it always fails now --------- Co-authored-by: Kevin Joiner <[email protected]>
aruiz14
added a commit
to aruiz14/wrangler
that referenced
this pull request
Jan 5, 2024
…nge (rancher#341) * Allow GeneratingHandlers to skip Apply if resource version didn't change * Run go generate * Add call count to FakeApply * Add test for UniqueApplyForResourceVersion * Simplify error handling Co-authored-by: Kevin Joiner <[email protected]> * Make generated code more readable Co-authored-by: Kevin Joiner <[email protected]> * Rename constant * Rename seenResourceVersion to storeResourceVersion * go generate * Add comments to code generator * Run go generate * Upgrade golangci-lint GitHub action, since it always fails now --------- Co-authored-by: Kevin Joiner <[email protected]>
aruiz14
added a commit
to aruiz14/wrangler
that referenced
this pull request
Jan 9, 2024
…nge (rancher#341) * Allow GeneratingHandlers to skip Apply if resource version didn't change * Run go generate * Add call count to FakeApply * Add test for UniqueApplyForResourceVersion * Simplify error handling Co-authored-by: Kevin Joiner <[email protected]> * Make generated code more readable Co-authored-by: Kevin Joiner <[email protected]> * Rename constant * Rename seenResourceVersion to storeResourceVersion * go generate * Add comments to code generator * Run go generate * Upgrade golangci-lint GitHub action, since it always fails now --------- Co-authored-by: Kevin Joiner <[email protected]>
aruiz14
added a commit
to aruiz14/wrangler
that referenced
this pull request
Jan 10, 2024
…nge (rancher#341) * Allow GeneratingHandlers to skip Apply if resource version didn't change * Run go generate * Add call count to FakeApply * Add test for UniqueApplyForResourceVersion * Simplify error handling Co-authored-by: Kevin Joiner <[email protected]> * Make generated code more readable Co-authored-by: Kevin Joiner <[email protected]> * Rename constant * Rename seenResourceVersion to storeResourceVersion * go generate * Add comments to code generator * Run go generate * Upgrade golangci-lint GitHub action, since it always fails now --------- Co-authored-by: Kevin Joiner <[email protected]>
aruiz14
added a commit
to aruiz14/wrangler
that referenced
this pull request
Jan 10, 2024
…nge (rancher#341) * Allow GeneratingHandlers to skip Apply if resource version didn't change * Run go generate * Add call count to FakeApply * Add test for UniqueApplyForResourceVersion * Simplify error handling Co-authored-by: Kevin Joiner <[email protected]> * Make generated code more readable Co-authored-by: Kevin Joiner <[email protected]> * Rename constant * Rename seenResourceVersion to storeResourceVersion * go generate * Add comments to code generator * Run go generate * Upgrade golangci-lint GitHub action, since it always fails now --------- Co-authored-by: Kevin Joiner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extends
GeneratingHandlerOptionsto allow a newUniqueApplyForResourceVersion, which will keep track of the latest resource version for each individual resource that was handled, avoiding subsequent calls to Apply if the first one was successful. The goal is to limit the number of Apply is called with the same inputs, reducing the computation costs. However, this means that handlers must be deterministic, in the sense that they must produce the same object list for the same input resource, hence this option must be carefully enabled on a case by case basis. It has no impact on other users if the option is not enabled.We have successfully tested this feature on Fleet's Bundle and GitRepo generating handlers (see rancher/fleet#1964 for an example usage), while trying to mitigate the effect of over-firing handlers (we are also working towards the root cause of the over-firing, which may be partially caused by the use of
Enqueue).