-
Notifications
You must be signed in to change notification settings - Fork 235
feat: ReconcileUtils for strongly consistent updates - alternative algorithm #3106
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
|
@shawkins I changed here the event filtering algorithm; one bit that bothered me with the one on Lines 106 to 109 in 589b7a6
Although in normal case this would not cause to much of a delay in event processing since the event from our update will arrive just after the request is sent. However considering the case when some does an update on the same resource just when we starten our filtering update this might lock the thread for the full duration for the request processing. So I created an alternative implementation (first version, to be refined soon) with our good old friend event recording. Pls take a look if you have the bandwidth, and let me know what do you think. thank you!! |
c710aa2 to
755f41b
Compare
That is correct. Until the update is complete, event processing is paused once you hit an event for the same resource. Part of the motivation was this allowed to keep much of the existing calling code structure.
There's a lot of changes here, so it may take a bit to fully review. Since we're really just trying to solve the case of the update event delivered prior to the completion of the update operation, things may be able to be simplified a bit. I put some ideas in #3109 Otherwise I think things look good. Having most of the handling localized to eventFilteringUpdateAndCacheResource will keep things more straight-forward. |
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
…gortihm 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]>
7b46752 to
be8f5f7
Compare
be8f5f7 to
630cde5
Compare
|
@shawkins something went wrong with this force push I will revert it here if not objections, could you pls propose changes in you branch? thx |
630cde5 to
be8f5f7
Compare
I'm sorry, I just noticed that I'm tracking the upstream branch for some reason. |
| // will receive | ||
| // additional event | ||
| filterEvent = false; | ||
| obsoleteEvent = comp == 0; |
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.
This is not right. Note that it is used only if there is no explicit filtering
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.
You can remove that commit from this pr. We can discuss on #3114
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.
np, just moved it back for now.
Signed-off-by: Attila Mészáros <[email protected]>
eaa50d1 to
793a56d
Compare
ReconciliationDispatcher)UpdateControl.patchStatus(and others), since it won't trigger the reconiliation for the event in that update.ReconcilerUtilstoReconcilerUtilsInternal, this is breaking but that utils was never advertised for non-internal usage