ARROW-4610: [Plasma] Avoid Crash in Plasma Java Client #3682
Conversation
guoyuhong
changed the title
praveenbingo
left a comment
praveenbingo
left a comment
There was a problem hiding this comment.
could you please add a test too..
|
@praveenbingo Thanks. I have added the test. The test output is: |
guoyuhong
force-pushed
the
removeCheckFromJNI
branch
from
fee4764 to
3cbb4e3
Compare
|
|
||
| cleanup(); | ||
| // Test calling shuntdown while getting the object. | ||
| Thread thread = new Thread(() -> { |
There was a problem hiding this comment.
for my understanding..is there any reason to call this in a separate thread..can we just call using cleanup?
There was a problem hiding this comment.
If we call cleanup directly, the socket is closed normally. There will be no exception as expected. This exception happens while client is waiting the reply from the server and at this time the server is killed or crashed.
There was a problem hiding this comment.
Ok, is this test is not susceptible on timing issues then? for e.g. if the read returns before the server cleanup command.
There was a problem hiding this comment.
Yes, if the read returns before the server cleanup command, this test will fail. However, I used a none-existent object id in the get function and the function timeout is 3 seconds to guarantee that this test will pass.
praveenbingo
left a comment
praveenbingo
left a comment
There was a problem hiding this comment.
+1
Thanks for doing this @guoyuhong
|
@pcmoritz can you please help merge this. |
|
@pitrou Could you help to merge this? |
| inline void throw_exception_if_not_OK(JNIEnv* env, const arrow::Status& status) { | ||
| if (!status.ok()) { | ||
| jclass Exception = | ||
| env->FindClass("org/apache/arrow/plasma/exceptions/PlasmaClientException"); |
There was a problem hiding this comment.
Ideally, we would raise different exceptions based on the status error category. I guess this is good enough for now.
|
@guoyuhong Will do. |
|
@pitrou Thanks! |
This PR removes all `ARROW_CHECK` from the JNI code to avoid Java client from crashing. The Java client will throw exception instead. For one thing, it is better to throw exceptions from the lower part and let the upper user to decide how to handle it. For another, JVM should not crash at all times and there are a lot of JVM core dump files when we are doing some failover tests by killing Plasma Server. Author: Yuhong Guo <[email protected]> Closes apache#3682 from guoyuhong/removeCheckFromJNI and squashes the following commits: 3cbb4e3 <Yuhong Guo> Add Plasma Client Java test 52c52bd <Yuhong Guo> Avoid Crash in Plasma Java Client
3 participants
This PR removes all
ARROW_CHECKfrom the JNI code to avoid Java client from crashing. The Java client will throw exception instead. For one thing, it is better to throw exceptions from the lower part and let the upper user to decide how to handle it. For another, JVM should not crash at all times and there are a lot of JVM core dump files when we are doing some failover tests by killing Plasma Server.