Skip to content
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.

Sign up for GitHub

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

[core] Remove grpc client thread number #49237

Merged
merged 3 commits into from
Dec 17, 2024
Merged

[core] Remove grpc client thread number #49237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d631f40
remove grpc client thread number
dentiny Dec 12, 2024
4ad71da
remove grpc client constructor
dentiny Dec 12, 2024
c080c61
remove callsite
dentiny Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions src/ray/rpc/grpc_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,25 +109,6 @@ class GrpcClient {
stub_ = GrpcService::NewStub(channel_);
}

GrpcClient(const std::string &address,
const int port,
ClientCallManager &call_manager,
int num_threads,
bool use_tls = false)
: client_call_manager_(call_manager), use_tls_(use_tls) {
grpc::ChannelArguments argument = CreateDefaultChannelArguments();
grpc::ResourceQuota quota;
quota.SetMaxThreads(num_threads);
argument.SetResourceQuota(quota);
argument.SetInt(GRPC_ARG_ENABLE_HTTP_PROXY,
::RayConfig::instance().grpc_enable_http_proxy() ? 1 : 0);
argument.SetMaxSendMessageSize(::RayConfig::instance().max_grpc_message_size());
argument.SetMaxReceiveMessageSize(::RayConfig::instance().max_grpc_message_size());

channel_ = BuildChannel(address, port, argument);
stub_ = GrpcService::NewStub(channel_);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

change places where this constructor is used too? Are the other uses casting num_threads to bool rn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't find it used anywhere via simple grep, but I will wait for CI results.

Copy link
Contributor

Choose a reason for hiding this comment

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

ray/src/ray/rpc/object_manager/object_manager_client.h

Lines 51 to 52 in 2385b18

grpc_clients_.emplace_back(new GrpcClient<ObjectManagerService>(
address, port, client_call_manager, num_connections_));
this seems to be the constructor used here, i think now it's just using one above this one instead and implicitly coverts the int to a bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we make sure we removed all usages of this constructor and didn't accidentally call the one above by converting int to bool?

Copy link
Contributor Author

@dentiny dentiny Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I made an extra test PR: #49283, which throws exception on the grpc client overload removed in this PR; and it turns out all CI tests pass.


/// Create a new `ClientCall` and send request.
///
/// \tparam Request Type of the request message.
Expand Down
4 changes: 2 additions & 2 deletions src/ray/rpc/object_manager/object_manager_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ class ObjectManagerClient {
freeobjects_rr_index_ = rand() % num_connections_;
grpc_clients_.reserve(num_connections_);
for (int i = 0; i < num_connections_; i++) {
grpc_clients_.emplace_back(new GrpcClient<ObjectManagerService>(
address, port, client_call_manager, num_connections_));
grpc_clients_.emplace_back(
new GrpcClient<ObjectManagerService>(address, port, client_call_manager));
}
};

Expand Down
Loading