-
Notifications
You must be signed in to change notification settings - Fork 229
feat: option to triggering reconciler on all events #2894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Conversation
c4f8193
to
eeff1ed
Compare
aa171b6
to
8c44cd5
Compare
2e5e220
to
8f69a5e
Compare
0f0cca9
to
fd68244
Compare
c1f2daa
to
1765745
Compare
As mentioned in the issue, I'd rather see this supported via listeners than via yet another feature flag. |
The idea behind this is same as controllers work in go, having a separate listernet would not be an eleant solution, basically we just trigger reconiliation for all the events as in go is done by default. This way we don't need an another interface or any other construct. |
Also in the past on purpose reduced the number of those interaces, and I think that was the right choice. I'm not that happy about to much feature flags, but I see this the as the better option now, since it is closed to go; feature flags use can much easier see than a new interface. And note that this is nost just about the delete events, but all the events, for example when others have finalizers added to the custom resources, it is marked for deletion, but meanwhile updated (or just on the mar of deletion event) so acutally much harder to capture with an interface. I really think this is a better model just to have to trigger the reconciler on everthing than having separate methods. |
Following the go SDK should only been done where it makes sense. I don't think it's a particularly compelling argument for this particular design or the other flags that have been introduced lately. |
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
9975e99
to
c4b5371
Compare
Reviewing again but could you please add comments / explanations on the integration test? |
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this LGTM
...java/io/javaoperatorsdk/operator/processing/event/source/controller/ResourceDeleteEvent.java
Outdated
Show resolved
Hide resolved
...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ResourceState.java
Outdated
Show resolved
Hide resolved
...work-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java
Outdated
Show resolved
Hide resolved
…tor/processing/event/ResourceState.java Co-authored-by: Martin Stefanko <[email protected]>
…tor/processing/event/source/controller/ResourceDeleteEvent.java Co-authored-by: Martin Stefanko <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Added comments / explanations for the tests. Thank you! I'm also thinking to later do an additional PR, with more samples and guides. |
Signed-off-by: Attila Mészáros <[email protected]>
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Show resolved
Hide resolved
...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/PrimaryUpdateAndCacheUtils.java
Outdated
Show resolved
Hide resolved
…tor/api/reconciler/PrimaryUpdateAndCacheUtils.java Co-authored-by: Martin Stefanko <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Thank you for the added comments, however, what I was after was more comments as to what the reconciliation logic is and what it's trying to show as this is the most important part to understand what the test is trying to achieve. |
* | ||
* @return true Delete event received for primary resource | ||
*/ | ||
boolean isPrimaryResourceDeleted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: add @SInCE here and below?
* By settings to true, reconcile method will be triggered on every event, thus even for Delete | ||
* event. You cannot use {@link Cleaner} or managed dependent resources in that case. See | ||
* documentation for further details. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: add @SInCE?
Adds option to trigger reconciliation on all events.
Signed-off-by: Attila Mészáros [email protected]