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.
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
Orchestrator implementation #53
Orchestrator implementation #53
Changes from 28 commits
124d83e
8c07603
ccf3a75
8fbde42
ee54b65
3b505db
071e8e5
0e853a8
6234b8a
fad0c13
7723d2c
78ecf04
2f15c34
cc6284e
24d87a6
aefa63b
3672040
4c05a5a
f302e8c
60f7a22
97c91e7
aeef7ca
5315967
d3d1e3b
6bcaf46
30246e7
ecb0a28
144d156
a36fc60
d08ddcc
4e4e0cc
57ff78e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
How often is this expected to happen? It looks like this channel is buffered as 1. if there is spam of updates, maybe it is fine to drop?
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.
There's more discussion for this here.
The idea behind this was that a downstream receiver could be blocked for any reason. Initially I had went with a timeout, but in discussions with @jyotimahapatra we realized that it would imply the slowest sidecar would block new updates from being sent to other sidecars, due to the use of WaitGroups. We can't get rid of WaitGroups because more recent updates might get overriden by a slower update if updates were bursty.
The buffer is necessary in order to give time for the receiver to initialize. I realized in this test that the second watch (for the same aggregated key) would fall to the
default
case if the channel was unbuffered.If there is a spam of updates, I think it's reasonable to drop since only one of those updates will be applied downstream anyway, but we do lose the the guarantee that it is the "latest" update. Could move back to the configured timeout model if this is a concern, but I put some of those downsides ^.
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.
My thoughts are to move forward with the default for now. I'll document this. If it is a problem in practice, then we can revisit the timeout model or other approaches.
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.
sounds good to me.
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.
Yeah in the future, you can do what Envoymanager does and drop the oldest and enqueue the newest rather than logging an error.
That has been working quite well for us since none of the configurations requires ordering and we send the state of the world. We always just want the latest update.