-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
ARROW-4236: [java] Distinct plasma client create exceptions #3306
Changes from all commits
f3d12a6
4710d59
5699eff
acc7c06
10ff110
c4fe54f
5586036
88e1702
3e512b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.arrow.plasma.exceptions; | ||
|
||
public class DuplicateObjectException extends RuntimeException { | ||
|
||
public DuplicateObjectException(String objectId) { | ||
super("An object with ID " + objectId + " already exists in the plasma store."); | ||
} | ||
|
||
public DuplicateObjectException(String objectId, Throwable t) { | ||
super("An object with ID " + objectId + " already exists in the plasma store.", t); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.arrow.plasma.exceptions; | ||
|
||
public class PlasmaOutOfMemoryException extends RuntimeException { | ||
|
||
public PlasmaOutOfMemoryException() { | ||
super("The plasma store ran out of memory."); | ||
} | ||
|
||
public PlasmaOutOfMemoryException(Throwable t) { | ||
super("The plasma store ran out of memory.", t); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,9 @@ | |
import java.util.concurrent.TimeUnit; | ||
import java.util.stream.Collectors; | ||
|
||
import org.apache.arrow.plasma.exceptions.DuplicateObjectException; | ||
import org.junit.Assert; | ||
|
||
public class PlasmaClientTest { | ||
|
||
private String storeSuffix = "/tmp/store"; | ||
|
@@ -142,8 +145,12 @@ public void doTest() { | |
assert Arrays.equals(values.get(0), value1); | ||
assert Arrays.equals(values.get(1), value2); | ||
System.out.println("Plasma java client get multi-object test success."); | ||
pLink.put(id1, value1, null); | ||
System.out.println("Plasma java client put same object twice exception test success."); | ||
try { | ||
pLink.put(id1, value1, null); | ||
Assert.fail("Fail to throw DuplicateObjectException when put an object into plasma store twice."); | ||
} catch (DuplicateObjectException e) { | ||
System.out.println("Plasma java client put same object twice exception test success."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you also want to assert that an exception is thrown.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
byte[] id1Hash = pLink.hash(id1); | ||
assert id1Hash != null; | ||
System.out.println("Plasma java client hash test success."); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
any reason to change the behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.