-
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] eagerly create router for handles held by proxy #47031
Conversation
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
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.
Would be nice to add a new test maybe try to update routes on start and see the differences with/ without this change. But code itself LGTM
# Eagerly instantiate the router for each handle so it can receive | ||
# the replica set from the controller. | ||
handle._get_or_create_router() |
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.
nit: would prefer that we add an argument to get_handle
instead of adding a private method call here
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, added _lazily_initialize_router
to get_deployment_handle
and DeploymentHandle
constructor. This is set to false in the proxy.
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.
@edoakes Removed the parameter for now because it causes the router to be initialized before options passed in .options()
are picked up by the initialized router, and there is no super clean way of adding the param and fixing that. Reverted to manually calling private method, will think more about how to add an official parameter for this.
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
I've added tests for the new parameter I added for get handle. As for testing this in the proxy, I'll be adding more comprehensive e2e tests once more changes are made in later PRs! |
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
6e6ffe7
to
2dd489c
Compare
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
c54373b
to
b094495
Compare
Why are these changes needed?
Eagerly create the router for handles held by the proxy actor, so that the controller can send the replica sets + their actor handles for each deployment upon proxy startup, instead of waiting for the first request to come in. This helps with improving controller fault tolerance.
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.