[core] Make PinObjectIDs RPC Fault Tolerant #56443
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: joshlee <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request makes the PinObjectIDs RPC fault-tolerant by switching to a retryable RPC client. To support this, new unit tests have been added to verify the idempotency of the HandlePinObjectIDs handler. The changes look correct and well-tested. My main feedback is to refactor the new tests to reduce code duplication.
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
dayshah
reviewed
Sep 12, 2025
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
Signed-off-by: joshlee <[email protected]>
dayshah
approved these changes
Sep 16, 2025
| buffers.second.size())}; | ||
| object_buffers->emplace_back(shm_buffer); | ||
| } else { | ||
| object_buffers->emplace_back(plasma::ObjectBuffer{}); |
Contributor
There was a problem hiding this comment.
why the change to emplace in the buffer even if the obj isn't there in fake plasma client?
Contributor
Author
There was a problem hiding this comment.
prod version does this as well:
ray/src/ray/object_manager/plasma/client.cc
Line 631 in 0c62bdb
by resizing the vector then skipping over indices that don't contain. Hence not emplacing caused issues with testing since it varied from the test causing RAY_CHECK failure here:
ray/src/ray/raylet/node_manager.cc
Line 2421 in 0c62bdb
TLDR: it uses these dummy default values to detect what objects are not in plasma
Contributor
There was a problem hiding this comment.
why do we need the change for this test though?
Contributor
Author
There was a problem hiding this comment.
The PinObjectIDs test also test the not happy path where we try to pin an object that doesn't exist which will trigger the RAY_CHECK in node_manager.cc I linked above
edoakes
reviewed
Sep 16, 2025
Signed-off-by: joshlee <[email protected]>
edoakes
approved these changes
Sep 17, 2025
zma2
pushed a commit
to zma2/ray
that referenced
this pull request
Sep 23, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: Zhiqiang Ma <[email protected]>
ZacAttack
pushed a commit
to ZacAttack/ray
that referenced
this pull request
Sep 24, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: zac <[email protected]>
elliot-barn
pushed a commit
that referenced
this pull request
Sep 24, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: elliot-barn <[email protected]>
marcostephan
pushed a commit
to marcostephan/ray
that referenced
this pull request
Sep 24, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: Marco Stephan <[email protected]>
elliot-barn
pushed a commit
that referenced
this pull request
Sep 27, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: elliot-barn <[email protected]>
dstrodtman
pushed a commit
to dstrodtman/ray
that referenced
this pull request
Oct 6, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]> Signed-off-by: Douglas Strodtman <[email protected]>
justinyeh1995
pushed a commit
to justinyeh1995/ray
that referenced
this pull request
Oct 20, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]>
landscapepainter
pushed a commit
to landscapepainter/ray
that referenced
this pull request
Nov 17, 2025
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency. --------- Signed-off-by: joshlee <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
Making PinObjectIDs RPC fault tolerant. Added cpp unit tests to verify idempotency.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.