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

Jump to bottom

[Core] Retryable grpc client #47981

Merged
merged 39 commits into from
Dec 3, 2024
Merged

[Core] Retryable grpc client #47981

merged 39 commits into from
Dec 3, 2024

Conversation

jjyao
Copy link
Collaborator

@jjyao jjyao commented Oct 10, 2024 •
edited
Loading

Why are these changes needed?

Currently gcs_rpc_client has retries for gcs rpc calls and this PR moves the retry functionality to RetryableGrpcClient so that it can be used by non-gcs rpc client (e.g. core worker client).

Also enable retry for ReportGeneratorItemReturns rpc since it's idempotent.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@jjyao
Retryable grpc client
b47dac0 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao jjyao added the go add ONLY when ready to merge, run all tests label Oct 11, 2024
jjyao added 3 commits October 10, 2024 23:10
@jjyao
up
3ead394 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao
up
e7fc94b 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao
up
e944542 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao
up
b072f62 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao
up
4bf5f52 
Signed-off-by: Jiajun Yao <[email protected]>
rynewang
rynewang reviewed Oct 12, 2024
View reviewed changes
src/ray/rpc/retryable_grpc_client.h Outdated Show resolved Hide resolved
src/ray/rpc/retryable_grpc_client.h Outdated Show resolved Hide resolved
src/ray/rpc/retryable_grpc_client.h Outdated Show resolved Hide resolved
src/ray/rpc/retryable_grpc_client.h Outdated Show resolved Hide resolved
src/ray/rpc/worker/core_worker_client.h Outdated Show resolved Hide resolved
src/ray/rpc/worker/core_worker_client.h Outdated Show resolved Hide resolved
src/ray/rpc/worker/core_worker_client.h Outdated Show resolved Hide resolved
@jjyao jjyao marked this pull request as ready for review October 19, 2024 04:26
@jjyao
comments
ed8051b 
Signed-off-by: Jiajun Yao <[email protected]>
@jjyao jjyao requested a review from rynewang October 19, 2024 04:37
@jjyao jjyao assigned jjyao and rynewang and unassigned rkooo567, rynewang and jjyao Oct 28, 2024
dentiny
dentiny reviewed Nov 19, 2024
View reviewed changes
src/ray/rpc/retryable_grpc_client.cc

RAY_CHECK(server_unavailable_timeout_time_.has_value());

auto status = channel_->GetState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment what is false.

@jjyao
up
381a626 
Signed-off-by: Jiajun Yao <[email protected]>
rynewang
rynewang approved these changes Nov 22, 2024
View reviewed changes
src/ray/rpc/retryable_grpc_client.h
* Status::TimedOut. If the whole client does not reconnect within
* server_unavailable_timeout_seconds, server_unavailable_timeout_callback is invoked.
*
* When all callers of the client release the shared_ptr of the client, the client
Copy link
Contributor

Choose a reason for hiding this comment

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

since the destruction logic is the same across all callsites, dtor is better than deleter

@jjyao
up
f90f6a5 
Signed-off-by: Jiajun Yao <[email protected]>
dentiny
dentiny reviewed Nov 23, 2024
View reviewed changes
src/ray/core_worker/core_worker.cc
@@ -763,7 +791,11 @@ void CoreWorker::Shutdown() {

// Now that gcs_client is not used within io service, we can reset the pointer and clean
// it up.
gcs_client_.reset();
if (gcs_client_) {
Copy link
Contributor

@dentiny dentiny Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I were you, I would place the disconnection logic into gcs_client_'s deleter.
The benefit of which is: we keep invariant when gcs client is valid, connection is guaranteed to be valid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave it for a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you plan to do that, maybe leave a TODO in case we forget?
Feel free to leave it under my name.

@jjyao jjyao assigned jjyao and unassigned rynewang Nov 25, 2024
dentiny
dentiny reviewed Nov 26, 2024
View reviewed changes
src/ray/gcs/gcs_server/test/gcs_server_test_util.h
@@ -321,6 +321,10 @@ struct GcsServerMocker {
drain_raylet_callbacks.push_back(callback);
};

void IsLocalWorkerDead(
Copy link
Contributor

Choose a reason for hiding this comment

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

This class should be named as Fake, not Mock...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR is already big. Let's do these cleanups in a separate PR.

dentiny
dentiny approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@dentiny dentiny left a comment

Choose a reason for hiding this comment

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

Chatted offline, most of my prev comments are caused by the wide-spread usage of shared pointer... Let's proceed, I will take another look

@jjyao jjyao merged commit 3570f3e into ray-project:master Dec 3, 2024
5 checks passed
@jjyao jjyao deleted the jjyao/rettttry branch December 3, 2024 18:54
simonsays1980 pushed a commit to simonsays1980/ray that referenced this pull request Dec 3, 2024
@jjyao @simonsays1980
[Core] Retryable grpc client (ray-project#47981)
12c4f91 
Signed-off-by: Jiajun Yao <[email protected]>
jecsand838 pushed a commit to jecsand838/ray that referenced this pull request Dec 4, 2024
@jjyao @jecsand838
[Core] Retryable grpc client (ray-project#47981)
11152e6 
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Connor Sanders <[email protected]>
dentiny pushed a commit to dentiny/ray that referenced this pull request Dec 7, 2024
@jjyao @dentiny
[Core] Retryable grpc client (ray-project#47981)
2ce43b6 
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: hjiang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynewang rynewang rynewang approved these changes

@dentiny dentiny dentiny approved these changes

Assignees

@jjyao jjyao

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jjyao @dentiny @rynewang @rkooo567