ignore object exists error for memory store provider

[zhijunfu]

Why are these changes needed?

It's reported that sometimes core worker test fails because we don't ignore the the object exists error for memory store provider. This PR fixes that issue.

https://travis-ci.com/ray-project/ray/jobs/229393241

Related issue number

Checks

• [Y] I've run scripts/format.sh to lint the changes in this PR.
• [Y] I've included any doc changes needed for https://ray.readthedocs.io/en/latest/.

[AmplabJenkins]

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

[edoakes]

Probably better to define a separate error type rather than checking the message. Especially considering that we should probably bubble the error up to the worker instead of hard-coding an ignore when there are duplicate keys. Don't have to do that in this PR but would be good to define the error type now.

[zhijunfu]

Thanks for reviewing. It probably depends on whether we should bubble up the error or just ignore it inside object interface, but yes I agree it's better to have a type instead of a message for this, thus I've added a new ObjectExists status.

[AmplabJenkins]

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

[edoakes]

We should definitely do it in the ObjectStoreInterface, it shouldn't be much added work and we can then remove the same logic from inside the PlasmaStoreProvider.

[zhijunfu]

Looking at the code again, I realized putting it into ObjectStoreInterface would not work in this case, because the memory store provider is used by the direct actor call transport, which calls the provider's Put directly, it won't call ObjectStoreInterface.

We should definitely do it in the ObjectStoreInterface, it shouldn't be much added work and we can then remove the same logic from inside the PlasmaStoreProvider.

[edoakes]

Why isn't the direct actor call transport going through the object interface? The object interface should know where to put the object based on the ID, right?

[zhijunfu]

Why isn't the direct actor call transport going through the object interface? The object interface should know where to put the object based on the ID, right?

Currently we use memory store just to contain the return objects of direct actor call tasks, nothing else.

When user calls ray.put to add an object into store, we assume he would use the object later and possibly from different machines, so we put the object into plasma in this case. That's why ObjectInterface's put always writes to plasma.

[AmplabJenkins]

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

[edoakes]

Hmm we should probably still go through the same codepath for this, otherwise we're going to end up with duplicated code/bugs (like this one). Shouldn't be much code or runtime overhead to switch based on the object ID - what do you think?

[zhijunfu]

The issue is about semantics & correctness, not overhead.

I think the assumption is when user directly calls ray.put, which in turns calls ObjectInterface's Put(), he is very likely to use this object later, and possibly pass this object id to another machine, then do a ray.get() there, so I want to make sure that ObjectInterface 's Put() will store the object to plasma and nowhere else. If it's put into memory store, then user is not able to get the object from a remote machine, I think that would surprise the user.

Try to do the switch based on the object ID, while also ensure the user's put will always be stored into plasma inside ObjectInterface would complicate the current logic, not simply it. I'd prefer to keep the current logic as is.

Hmm we should probably still go through the same codepath for this, otherwise we're going to end up with duplicated code/bugs (like this one). Shouldn't be much code or runtime overhead to switch based on the object ID - what do you think?

[edoakes]

I see your point, but I would find it very strange to have Get() implement multiple storage providers but not Put(). I also don't think it would complicate the logic very much given that we already have the code to switch based on the storage provider - we can re-use that. Should just be one additional line in Put().

[zhijunfu]

If you really want to do it that way, could you please file another PR? You can assign me as reviewer. thanks.

I see your point, but I would find it very strange to have Get() implement multiple storage providers but not Put(). I also don't think it would complicate the logic very much given that we already have the code to switch based on the storage provider - we can re-use that. Should just be one additional line in Put().

[raulchen]

@edoakes I think the refactor can be done in a separate PR. Let's merge this one first to unblock CI.

[edoakes]

Sure, I'm not completely sold on any solution yet. Please go ahead and merge.

…

On Wed, Sep 4, 2019, 20:35 Hao Chen ***@***.***> wrote: @edoakes <https://github.com/edoakes> I think the refactor can be done in a separate PR. Let's merge this one first to unblock CI. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5607?email_source=notifications&email_token=ACLKAZPE72POPIN6LKW4ODTQIB5BHA5CNFSM4ISVOJ62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD55WYXA#issuecomment-528182364>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLKAZI3LEMAJCPA26SZHSDQIB5BHANCNFSM4ISVOJ6Q> .