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][Object Store]Fix object reconstruction hang on arguments pending creation. #47607

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[Core][Object Store]Fix object reconstruction hang on arguments pending creation. #47607

wants to merge 2 commits into from

Conversation

Catch-Bull
Copy link
Contributor

Why are these changes needed?

@jjyao @rynewang Do you have any other concerns about this fix? If you generally agree on this PR, I will add the UT later and start the formal review.

Related issue number

Closes #47606

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 :(

hejialing.hjl added 2 commits September 11, 2024 20:56
save
eb97a5d 
Signed-off-by: hejialing.hjl <hejialing.hjl@bytedance.com>
save
d52a62e 
Signed-off-by: hejialing.hjl <hejialing.hjl@bytedance.com>
@Catch-Bull Catch-Bull force-pushed the fix_streaming_object_reconstruct_hang branch from debd360 to d52a62e Compare September 11, 2024 13:01
@Catch-Bull Catch-Bull added the go add ONLY when ready to merge, run all tests label Sep 11, 2024
jjyao
jjyao reviewed Sep 12, 2024
View reviewed changes
src/ray/core_worker/reference_count.cc
Comment on lines +1361 to +1364
if (pending_creation) {
RAY_LOG(WARNING) << "Object[" << object_id << "] has already been "
<< "marked as failed and cannot continue pending creation";
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should never happen?

src/ray/core_worker/core_worker.cc
RAY_CHECK_OK(PutInLocalPlasmaStore(object, object_id, /*pin_object=*/true));
if (mark_object_failed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need to have mark_object_failed boolean: we can always set pending_creation to false whenever put_in_local_plasma_callback is called.

src/ray/core_worker/reference_count.h
@@ -811,6 +813,8 @@ class ReferenceCounter : public ReferenceCounterInterface,

/// Whether or not this object was spilled.
bool did_spill = false;

bool mark_object_failed = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we need this.

@Catch-Bull
Copy link
Contributor Author

@jjyao The cause of this issue is that when the actor task is resubmitted, it is actually because the actor has already failed, which prevents the task from being dispatched. However, TaskManager::ResubmitTask does not perform a check and still returns true, causing the pending creation set here to never actually be set to false. This fix will also work.

The reason I chose to add mark_object_failed in Reference is that I believe it's a safer approach. I'm not quite sure if there might be other reasons that could lead to potential leaks in pending creation.

@jjyao
Copy link
Collaborator

jjyao commented Sep 13, 2024

@Catch-Bull I'm trying a different fix #47645. PTAL

@anyscalesam anyscalesam added @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. core Issues that should be addressed in Ray Core P1 Issue that should be fixed within a few weeks triage Needs triage (eg: priority, bug/not-bug, and owning component) and removed P1 Issue that should be fixed within a few weeks labels Sep 16, 2024
@jjyao jjyao added P0 Issues that should be fixed in short order and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjyao jjyao jjyao left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Catch-Bull Catch-Bull

Labels
core Issues that should be addressed in Ray Core @external-author-action-required Alternate tag for PRs where the author doesn't have labeling permission. go add ONLY when ready to merge, run all tests P0 Issues that should be fixed in short order
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Core][Object Store] During the object reconstruction, it is hang on arguments pending creation.
3 participants
@Catch-Bull @jjyao @anyscalesam