ARROW-4236: [java] Distinct plasma client create exceptions #3306

bibabolynn · 2019-01-04T06:22:00Z

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

raulchen · 2019-01-04T07:40:19Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

-                  "The plasma store ran out of memory and could not create this object.");
+    jclass Exception = env->FindClass(
+          "org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException");
+        env->ThrowNew(Exception, oid.hex().c_str());


PlasmaOutOfMemoryException doesn't take object id as the parameter.

raulchen · 2019-01-04T07:45:36Z

java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java

-
+  public void put(byte[] objectId, byte[] value, byte[] metadata)
+          throws DuplicateObjectException, PlasmaOutOfMemoryException {
+    ByteBuffer buf = PlasmaClientJNI.create(threadConn.get(), objectId, value.length, metadata);


what is threadConn?

raulchen · 2019-01-04T07:46:06Z

java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaOutOfMemoryException.java

+public class PlasmaOutOfMemoryException extends RuntimeException {
+
+  public PlasmaOutOfMemoryException () {
+    super("The plasma store ran out of memory and could not create object with ID.");


remove and could not create object with ID

raulchen · 2019-01-04T07:46:36Z

java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaOutOfMemoryException.java

+    super("The plasma store ran out of memory and could not create object with ID.", t);
+  }
+
+}


raulchen · 2019-01-04T07:46:50Z

java/plasma/src/main/java/org/apache/arrow/plasma/DuplicateObjectException.java

+    super("An object with ID " + objectId + " already exists in the plasma store.", t);
+  }
+
+}


praveenbingo · 2019-01-04T11:45:50Z

cpp/src/plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc

-                  "An object with this ID already exists in the plasma store.");
+    jclass Exception = env->FindClass(
+              "org/apache/arrow/plasma/exceptions/DuplicateObjectException");
+            env->ThrowNew(Exception, oid.hex().c_str());


indentation looks off.. also exception? for name..

praveenbingo · 2019-01-04T11:51:34Z

java/plasma/src/main/java/org/apache/arrow/plasma/PlasmaClient.java

-
+  public void put(byte[] objectId, byte[] value, byte[] metadata)
+          throws DuplicateObjectException, PlasmaOutOfMemoryException {
+    ByteBuffer buf = PlasmaClientJNI.create(conn, objectId, value.length, metadata);


this looks backward incompatible..earlier we were ignoring any exceptions while creating the buffer

which is why the test is failing here -

arrow/java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java

Line 145 in 41a4e72

pLink.put(id1, value1, null);

any reason to change the behavior?

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.

bibabolynn · 2019-01-08T04:16:58Z

@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.

raulchen · 2019-01-10T04:28:22Z

The CI failure should be unrelated. Because this PR only touches Java, but the failed tests are all Python.
@praveenbingo do you have any other questions about this PR?

praveenbingo

also looks like the python tests are having core dumps..would you be able to run the python tests to see if they are caused by this change
(i am not an expert in plasma to comment authoritatively)

praveenbingo · 2019-01-10T11:53:24Z

java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java

+    try {
+      pLink.put(id1, value1, null);
+    } catch (DuplicateObjectException e) {
+      System.out.println("Plasma java client put same object twice exception test success.");


do you also want to assert that an exception is thrown..

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

wesm · 2019-01-10T15:57:29Z

Can you please create a JIRA issue for this and update the PR title?

guoyuhong · 2019-01-11T06:35:50Z

https://issues.apache.org/jira/browse/ARROW-4236

bibabolynn · 2019-01-11T06:47:53Z

@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

pcmoritz

The change looks good to me.

pcmoritz · 2019-01-11T07:00:27Z

@bibabolynn @raulchen Are there any changes necessary in Ray for this?

pcmoritz · 2019-01-11T07:04:14Z

@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

I added you as contributor and assigned the Issue to you.

bibabolynn · 2019-01-11T07:26:26Z

@bibabolynn @raulchen Are there any changes necessary in Ray for this?

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

bibabolynn · 2019-01-14T07:55:33Z

@wesm @pcmoritz @praveenbingo since this pr has already been approved, I wonder when will it be merged?

raulchen · 2019-01-24T02:23:33Z

@praveenbingo @wesm any other actions needed from our side?

wesm · 2019-01-24T02:39:51Z

@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

praveenbingo · 2019-01-24T05:08:05Z

Change looks good to me too..

Please rebase to re-trigger and verify the tests and this should be good for merge, as @wesm said @pcmoritz can merge the change post the tests..

raulchen · 2019-01-24T06:02:59Z

All tests are passing. @pcmoritz could you help merge it when you have time? Thanks!

kou · 2019-01-24T06:03:58Z

I re-run CI. Now, CI is green.

pcmoritz · 2019-01-24T06:07:19Z

Yes, merging it, thanks for the contribution! Originally I left it open in case there are more comments.

bibabolynn closed this Jan 4, 2019

bibabolynn reopened this Jan 4, 2019

bibabolynn mentioned this pull request Jan 4, 2019

[java] when put an object in plasma store, ignore "object alreay exists" exception ray-project/ray#3687

Merged

raulchen reviewed Jan 4, 2019

View reviewed changes

praveenbingo reviewed Jan 4, 2019

View reviewed changes

praveenbingo reviewed Jan 10, 2019

View reviewed changes

bibabolynn changed the title ~~distinct plasma client create exceptions~~ ARROW-4236: [java] Distinct plasma client create exceptions Jan 11, 2019

pcmoritz approved these changes Jan 11, 2019

View reviewed changes

lynn and others added 9 commits January 24, 2019 12:00

distinct plasma client create exception

f3d12a6

fix

4710d59

blank line

5699eff

indentation

acc7c06

plasmaClientTest catch duplicate object exception

10ff110

cpp lint

c4fe54f

cpp lint

5586036

cpp lint

88e1702

add assert

3e512b6

bibabolynn force-pushed the dev_plasmaClientException branch from 7c9eb0a to 3e512b6 Compare January 24, 2019 04:00

pcmoritz closed this in 3405cd4 Jan 24, 2019

pcmoritz mentioned this pull request Feb 1, 2019

Use one memory mapped file for plasma ray-project/ray#3871

Merged

guoyuhong deleted the dev_plasmaClientException branch February 18, 2019 06:08

asfimport mentioned this pull request Jan 24, 2019

[JAVA] Distinct plasma client create exceptions #20815

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-4236: [java] Distinct plasma client create exceptions #3306

ARROW-4236: [java] Distinct plasma client create exceptions #3306

bibabolynn commented Jan 4, 2019

raulchen Jan 4, 2019

raulchen Jan 4, 2019

raulchen Jan 4, 2019

raulchen Jan 4, 2019

raulchen Jan 4, 2019

praveenbingo Jan 4, 2019

praveenbingo Jan 4, 2019

raulchen Jan 4, 2019

bibabolynn commented Jan 8, 2019

raulchen commented Jan 10, 2019

praveenbingo left a comment

praveenbingo Jan 10, 2019

bibabolynn Jan 10, 2019

wesm commented Jan 10, 2019

guoyuhong commented Jan 11, 2019

bibabolynn commented Jan 11, 2019

pcmoritz left a comment

pcmoritz commented Jan 11, 2019

pcmoritz commented Jan 11, 2019

bibabolynn commented Jan 11, 2019

bibabolynn commented Jan 14, 2019

raulchen commented Jan 24, 2019

wesm commented Jan 24, 2019

praveenbingo commented Jan 24, 2019

raulchen commented Jan 24, 2019

kou commented Jan 24, 2019

pcmoritz commented Jan 24, 2019 •

edited

Loading

ARROW-4236: [java] Distinct plasma client create exceptions #3306

ARROW-4236: [java] Distinct plasma client create exceptions #3306

Conversation

bibabolynn commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bibabolynn commented Jan 8, 2019

raulchen commented Jan 10, 2019

praveenbingo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm commented Jan 10, 2019

guoyuhong commented Jan 11, 2019

bibabolynn commented Jan 11, 2019

pcmoritz left a comment

Choose a reason for hiding this comment

pcmoritz commented Jan 11, 2019

pcmoritz commented Jan 11, 2019

bibabolynn commented Jan 11, 2019

bibabolynn commented Jan 14, 2019

raulchen commented Jan 24, 2019

wesm commented Jan 24, 2019

praveenbingo commented Jan 24, 2019

raulchen commented Jan 24, 2019

kou commented Jan 24, 2019

pcmoritz commented Jan 24, 2019 • edited Loading

pcmoritz commented Jan 24, 2019 •

edited

Loading