-
Notifications
You must be signed in to change notification settings - Fork 235
possible refinements #3109
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
possible refinements #3109
Conversation
|
|
||
| private int activeUpdates = 0; | ||
| private ResourceEvent lastEvent; | ||
| private int lastUpdatedResourceVersion = -1; |
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.
I don't think we can generally guarentee that the resource versions will fall in the int range.
| int comp = 0; | ||
| if (cached != null) { | ||
| comp = ReconcileUtils.validateAndCompareResourceVersions(resource, cached); | ||
| if (comp >= 0 || unknownState) { |
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.
@csviri can you explain why this allows for equality?
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.
Since we can remove the resource in that case from the cache.
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.
Right, so this needs refined a little further - it should remove, but still be seen as a known event.
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.
Note that in the implementation we strictly define two cases: when we want to filter the event and when we don't, based on user's intention.
So there is no grey area now.
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.
The wording may be confusing across a couple of methods.
The boolean used to mean "known event" - so if I put a version into the temporary resource cache, then see the event with the same version, it's a known event.
Does "filter event" have a subtely different meaning from "known event"?
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.
Hmm good question, the filter event mean to express that the event won't be propagated further (if true). This is bit more descriptive since we might actually not know all the events (watch connection issues).
7b46752 to
be8f5f7
Compare
ef2642a to
df5ade4
Compare
be8f5f7 to
630cde5
Compare
630cde5 to
be8f5f7
Compare
df5ade4 to
f414063
Compare
A few ideas for simplifying the logic.