Skip to content

Move plasma retry logic into plasma store provider#7328

Merged
edoakes merged 6 commits intoray-project:masterfrom
edoakes:plasma-retry
Feb 27, 2020
Merged

Move plasma retry logic into plasma store provider#7328
edoakes merged 6 commits intoray-project:masterfrom
edoakes:plasma-retry

Conversation

@edoakes
Copy link
Copy Markdown
Collaborator

@edoakes edoakes commented Feb 26, 2020

Why are these changes needed?

Standardizes the behavior in all of the places in the code that we create objects in plasma. Will also be necessary to trigger global GC (see #7327).

Checks

@AmplabJenkins
Copy link
Copy Markdown

Can one of the admins verify this patch?

@edoakes
Copy link
Copy Markdown
Collaborator Author

edoakes commented Feb 26, 2020

Wasn't sure how to add a test for this, but verified locally that it works for both ray.put() and task returns.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22417/
Test FAILed.

} else if (plasma::IsPlasmaObjectExists(plasma_status)) {
RAY_LOG(WARNING) << "Trying to put an object that already existed in plasma: "
<< object_id << ".";
status = Status::OK();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this probably isn't needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed but don't want to change it in this PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I just meant the status = ... line, but yeah I guess the whole block could be removed.

Copy link
Copy Markdown
Contributor

@stephanie-wang stephanie-wang left a comment

Choose a reason for hiding this comment

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

A couple small comments, but looks good!

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22437/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22440/
Test FAILed.

@AmplabJenkins
Copy link
Copy Markdown

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22439/
Test PASSed.

@AmplabJenkins
Copy link
Copy Markdown

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/22450/
Test FAILed.

@edoakes edoakes merged commit 2ad9bc5 into ray-project:master Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants