Skip to content

Conversation

@zcin
Copy link
Contributor

@zcin zcin commented Nov 12, 2024

Why are these changes needed?

  • Combine the endpoint router and the longest prefix router. The two differed in that one used match_route to get a handle, and one used get_handle_for_endpoint to get a handle (for HTTP and gRPC respectively). We can just have a single proxy router class that have both of these methods.
    • This is nice in that we only have one cached set of handles.
  • Add a default get_proxy_handle function in default_impl.py, which can be used to control how a handle is fetched and how it is initialized.
  • Infer request protocol from request context, which should deterministically indicate whether the request is an http request or a gRPC request. This then removes the need to set request protocol in the handle, which makes sense since request protocol is more metadata than handle configuration.

Related issue number

zcin added 4 commits November 12, 2024 11:42
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]>
@zcin zcin requested review from GeneDer and edoakes November 12, 2024 21:17
@zcin zcin added the go add ONLY when ready to merge, run all tests label Nov 12, 2024
zcin added 7 commits November 12, 2024 13:29
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]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Copy link
Member

@GeneDer GeneDer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

self.http_proxy.update_routes(endpoints)
if self.grpc_proxy is not None:
self.grpc_proxy.update_routes(endpoints)
self.proxy_router.update_routes(endpoints)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh this is nice! 👍

@zcin zcin merged commit dcf4892 into ray-project:master Nov 13, 2024
5 checks passed
@zcin zcin deleted the proxy-router-clean branch November 13, 2024 16:43
JP-sDEV pushed a commit to JP-sDEV/ray that referenced this pull request Nov 14, 2024
## Why are these changes needed?

- Combine the endpoint router and the longest prefix router. The two
differed in that one used `match_route` to get a handle, and one used
`get_handle_for_endpoint` to get a handle (for HTTP and gRPC
respectively). We can just have a single proxy router class that have
both of these methods.
  - This is nice in that we only have one cached set of handles.
- Add a default `get_proxy_handle` function in `default_impl.py`, which
can be used to control how a handle is fetched and how it is
initialized.
- Infer request protocol from request context, which should
deterministically indicate whether the request is an http request or a
gRPC request. This then removes the need to set request protocol in the
handle, which makes sense since request protocol is more metadata than
handle configuration.


Signed-off-by: Cindy Zhang <[email protected]>
mohitjain2504 pushed a commit to mohitjain2504/ray that referenced this pull request Nov 15, 2024
## Why are these changes needed?

- Combine the endpoint router and the longest prefix router. The two
differed in that one used `match_route` to get a handle, and one used
`get_handle_for_endpoint` to get a handle (for HTTP and gRPC
respectively). We can just have a single proxy router class that have
both of these methods.
  - This is nice in that we only have one cached set of handles.
- Add a default `get_proxy_handle` function in `default_impl.py`, which
can be used to control how a handle is fetched and how it is
initialized.
- Infer request protocol from request context, which should
deterministically indicate whether the request is an http request or a
gRPC request. This then removes the need to set request protocol in the
handle, which makes sense since request protocol is more metadata than
handle configuration.

Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: mohitjain2504 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-backlog go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants