-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Config: Implement deferred clusters on worker. #28702
Conversation
workers inline when there's traffic for that cluster. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
/assign @adisuissa |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
might make sense for adip to do api shepparding as well since he'll do the review. /unassign @mattklein123 |
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
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.
Thanks for working on this!
Overall this LGTM. Left a few comments, mostly minor.
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.
Thanks! Overall looks good.
I suggest adding some more comments explaining why some invariants hold.
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
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.
Thanks for the reviews @adisuissa !
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.
Thanks, LGTM!
/lgtm api
/assign-from @envoyproxy/senior-maintainers
@envoyproxy/senior-maintainers assignee is @mattklein123 |
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.
Nice!
Commit Message: Config: Implement deferred clusters on worker.
Additional Description: We initialize certain cluster types on workers inline when there's traffic for that cluster. This is a memory optimization for large envoy deployments with many clusters and workers. This will be followed up with an "eviction PR" so that the deferral benefits don't go away the longer the envoy process runs.
Risk Level: med / high
Testing: unit, integration
Docs Changes: included
Release Notes: included
Platform Specific Features: na
Runtime guard: Config guarded
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]