Move gcs_callback_types.h to rpc_callback_types.h #58596
Conversation
sunsetxh
force-pushed
the
br_grpc_callback
branch
from
c2c9de7 to
63261b9
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors gcs_callback_types.h to grpc_callback_types.h, making the callback types more generic by removing the gcs namespace. The changes across the codebase correctly reflect this rename and namespace removal.
My main feedback is to update the comments within the newly renamed grpc_callback_types.h to remove specific references to "GCS", aligning them with the goal of making these types more generic. This will improve code clarity and maintainability.
sunsetxh
changed the title
|
The callbacks in that file are not specific to gRPC. To avoid leaking implementation details of the rpc layer, let's call this |
|
@edoakes Thinks. I'll fix it soon. |
|
Thanks! Could you fix this comment as well |
sunsetxh
force-pushed
the
br_grpc_callback
branch
2 times, most recently
from
04821bd to
14586e3
Compare
ray-gardener
bot
added
core
sunsetxh
force-pushed
the
br_grpc_callback
branch
3 times, most recently
from
fbaaccc to
87d8331
Compare
|
I converted it to draft since it's still wip. |
sunsetxh
force-pushed
the
br_grpc_callback
branch
3 times, most recently
from
7dcfe82 to
f48d26d
Compare
sunsetxh
force-pushed
the
br_grpc_callback
branch
2 times, most recently
from
f0c56ca to
74af556
Compare
sunsetxh
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /// during the whole lifetime of client. | ||
| std::unique_ptr<rpc::ClientCallManager> client_call_manager_; | ||
| std::unique_ptr<rpc::RayletClient> raylet_client_; | ||
| InstrumentedIOContextWithThread io_context_; |
There was a problem hiding this comment.
It's different from the original design. io_context_ used to be static, so all raylet clients share one io_context_. Now each raylet client creates their own. Consider static it and initialize it only once
Signed-off-by: yuchao-wang <wyc984872769@hotmail.com>
sunsetxh
force-pushed
the
br_grpc_callback
branch
from
74af556 to
e00d99b
Compare
| /// during the whole lifetime of client. | ||
| std::unique_ptr<rpc::ClientCallManager> client_call_manager_; | ||
| std::unique_ptr<rpc::RayletClient> raylet_client_; | ||
| inline static InstrumentedIOContextWithThread io_context_{"raylet_client_io_service"}; |
There was a problem hiding this comment.
Bug: Static member changes io_context initialization timing
The io_context_ was changed from a function-local static to a class-level inline static member. While both create a singleton, the initialization timing differs significantly: function-local static uses lazy initialization (thread starts on first use), whereas class-level inline static uses eager initialization (thread starts during static initialization, before main()). Since InstrumentedIOContextWithThread spawns a thread in its constructor, this change causes a background thread to start before the program's main() function runs, which could cause issues with static initialization order or when the code is used from shared libraries. As noted by the reviewer, this differs from the original design intent.
Additional Locations (1)
jjyao
changed the title
jjyao
left a comment
jjyao
left a comment
There was a problem hiding this comment.
LG. Thanks for the contribution!
| // This is to avoid creating multiple threads for multiple clients in python. | ||
| static InstrumentedIOContextWithThread io_context("raylet_client_io_service"); | ||
| instrumented_io_context &io_service = io_context.GetIoService(); | ||
| instrumented_io_context &io_service = io_context_.GetIoService(); |
There was a problem hiding this comment.
Why changing this, seems unrelated to this PR
There was a problem hiding this comment.
Ah @sunsetxh is fixing your old comments #57004 (comment)
4 participants
Description
Move gcs_callback_types.h to rpc_callback_types.h
Related issues
Closes #58597