ARROW-4236: [java] Distinct plasma client create exceptions #3306
Conversation
There was a problem hiding this comment.
PlasmaOutOfMemoryException doesn't take object id as the parameter.
There was a problem hiding this comment.
remove and could not create object with ID
There was a problem hiding this comment.
indentation looks off.. also exception? for name..
There was a problem hiding this comment.
this looks backward incompatible..earlier we were ignoring any exceptions while creating the buffer
which is why the test is failing here - https://github.com/apache/arrow/blob/41a4e72f08021a682426ceb46c7921ad91a269cd/java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java#L145
any reason to change the behavior?
There was a problem hiding this comment.
The reason is because it makes more sense to propagate the exception to user code, and let user decide what to do. E.g., when putting an object with existing ID, someone may want to silently ignore the error, someone may want to use a different ID, and someone may want to terminate the program. The decision should depends on application logic.
|
@praveenbingo could you help check the ci result. I can't see why it failed or does it have anything to do with my changes. I've tried several times, but they all failed. |
|
The CI failure should be unrelated. Because this PR only touches Java, but the failed tests are all Python. |
There was a problem hiding this comment.
do you also want to assert that an exception is thrown..
There was a problem hiding this comment.
ok, I've added an assert. As for python tests, I've only changed java and jni files which won't have any influence on python code since python code won't reach these code in any cases. So I don't think ci failed because of my change
|
Can you please create a JIRA issue for this and update the PR title? |
bibabolynn
changed the title
|
@wesm I've asked somebody else to create the JIRA issue and paste the link above. Because I don't have authority. 'bibabolynn' is my account, maybe you can add me |
|
@bibabolynn @raulchen Are there any changes necessary in Ray for this? |
I added you as contributor and assigned the Issue to you. |
yes, there are changes in ray related to this which I will deal with after this pr is merged. That ray pr depends on this pr |
|
@wesm @pcmoritz @praveenbingo since this pr has already been approved, I wonder when will it be merged? |
|
@praveenbingo @wesm any other actions needed from our side? |
|
@pcmoritz is also a committer so he can also merge patches. Can you please rebase to make sure the build is passing? We fixed a bunch of problems that were going on in CI a couple weeks ago |
bibabolynn
force-pushed
the
dev_plasmaClientException
branch
from
7c9eb0a to
3e512b6
Compare
|
All tests are passing. @pcmoritz could you help merge it when you have time? Thanks! |
|
I re-run CI. Now, CI is green. |
|
Yes, merging it, thanks for the contribution! Originally I left it open in case there are more comments. |
when ray puts an object in plasma store, there are 2 exceptions may be thrown, one is "An object with this ID already exists in the plasma store" and the other is "The plasma store ran out of memory and could not create this object", distinct them rather than let them both be the same java class @raulchen please help review Author: yl187661 <[email protected]> Author: lynn <[email protected]> Closes apache#3306 from bibabolynn/dev_plasmaClientException and squashes the following commits: 3e512b6 <yl187661> add assert 88e1702 <yl187661> cpp lint 5586036 <yl187661> cpp lint c4fe54f <yl187661> cpp lint 10ff110 <yl187661> plasmaClientTest catch duplicate object exception acc7c06 <yl187661> indentation 5699eff <yl187661> blank line 4710d59 <yl187661> fix f3d12a6 <lynn> distinct plasma client create exception
7 participants
when ray puts an object in plasma store, there are 2 exceptions may be thrown, one is "An object with this ID already exists in the plasma store" and the other is "The plasma store ran out of memory and could not create this object", distinct them rather than let them both be the same java class
@raulchen please help review