-
Notifications
You must be signed in to change notification settings - Fork 5.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
[serve] Remove log poll client from replicas #19145
[serve] Remove log poll client from replicas #19145
Conversation
…ve-backend-config-long-poll-from-replica
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 a user-visible change because this option was not able to be parametrized anyways.
I actually think we should not bake shutdown time to constants, rather, just let the replica take backend config in the init args.
It should be parameterized option because it is useful adjust this parameter properly (not through monkey patching constants) for various testing, release testings, and fault injection testing. Additionally, this will have impact for how long can a replica scale down (relevant to autoscaling). We just need to do little work to make sure it's configurable (and remove experimental now we finalized the mechanism).
…ve-backend-config-long-poll-from-replica
@simon-mo fair point, let me refactor and make it a hidden field in |
@simon-mo updated |
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.
LGTM. You might need to fix Java compilation failures.
…ve-backend-config-long-poll-from-replica
…ve-backend-config-long-poll-from-replica
Why are these changes needed?
In general, broadcasting changes to the replicas via the LongPollClient is hard to reason about (it circumvents our versioning semantics as there's no rolling update). Ideally we would only be using the LongPollClient to broadcast replica membership and nothing else.
Related issue number
Closes #19145
Checks
scripts/format.sh
to lint the changes in this PR.