feat: use a predicate to select the custom resource for which a reconcile should be triggered #454
Conversation
| eventBuffer.addEvent(event); | ||
| executeBufferedEvents(event.getRelatedCustomResourceUid()); | ||
|
|
||
| if (event.getRelatedCustomResourceUid() != null) { |
There was a problem hiding this comment.
Can we replace this by creating a UID predicate so that we handle everything the same way?
There was a problem hiding this comment.
Absolutely, the reason I left is that it could be slightly more efficient as it would avoid an extra lookup but I will fix it.
| } finally { | ||
| lock.unlock(); | ||
| } | ||
| } | ||
|
|
||
| private void executeBufferedEvents(List<String> customResourceUids) { |
There was a problem hiding this comment.
Both versions of the method should be merged into a single one, imo.
There was a problem hiding this comment.
this is a leftover, I'll remove it
| import java.util.function.Predicate; | ||
|
|
||
| @SuppressWarnings("rawtypes") | ||
| public class DefaultEvent extends AbstractEvent { |
There was a problem hiding this comment.
Do we really need this class?
There was a problem hiding this comment.
We don't but I found a little bit annoying that AbstractEvent is declared abstract but in fact it does not have any abstract method so if you want to create a generic event, you'd need an anonymous class.
There was a problem hiding this comment.
I'll refactor AbstractEvent separately.
| .hasSize(2) | ||
| .allSatisfy( | ||
| s -> { | ||
| assertThat(s.getEvents()).isNotEmpty().hasOnlyElementsOfType(DefaultEvent.class); |
There was a problem hiding this comment.
Shouldn't we rather check that the events match cr1 and cr3 instead?
…cile should be triggered operator-framework#430
| @@ -163,6 +165,16 @@ public CustomResourceCache getCache() { | |||
| return getCache().getLatestResource(customResourceUid); | |||
| } | |||
|
|
|||
| // todo: remove | |||
There was a problem hiding this comment.
Why are these methods marked as todo: remove?
There was a problem hiding this comment.
copied from the other methods that were there, I don't know what was the rationale of the old comments, should I remove the comment or you have a plan to remove thos methods ?
There was a problem hiding this comment.
OK, I'll try to figure out what's going on there… :)
| public void setup() { | ||
| defaultEventHandler.setEventSourceManager(defaultEventSourceManagerMock); | ||
|
|
||
| // todo: remove |
|
Thank you! |
Fixes #430