ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java #7815
Conversation
|
@wesm Could you take a review? |
| @@ -22,11 +22,11 @@ | |||
| */ | |||
| public class PlasmaOutOfMemoryException extends RuntimeException { | |||
|
|
|||
| public PlasmaOutOfMemoryException() { | |||
| public PlasmaOutOfMemoryException(String message) { | |||
There was a problem hiding this comment.
Shouldn't we pass the string somewhere? eg add it to the original message?
There was a problem hiding this comment.
In plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc
Line 123
env->ThrowNew(exceptionClass, "");
Here passed an empty String.But There is no String arg in PlasmaOutOfMemoryException constructor.
And if we need to catch this exception, it would show java (Ljava/lang/String;Ljava/lang/String;I)V
So add a String arg in PlasmaOutOfMemoryException constructors.
There was a problem hiding this comment.
An unit test added, please take a review.
@rymurr
There was a problem hiding this comment.
thanks for the clarity @offthewall123
I think it would be better to add new constructors to augment the existing one and to append/replace the String message to the internal message.
There was a problem hiding this comment.
Thanks for reply. @rymurr
Yes, we can add a new constructor like public PlasmaOutOfMemoryException(String message) and keep the origin two.
But In plasma/lib/java/org_apache_arrow_plasma_PlasmaClientJNI.cc
line 123 env->ThrowNew(exceptionClass, ""); here just passed an empty String, and i searched seems no overloaded function like env->ThrowNew(exceptionClass) which not to pass a String arg.
If we replace String message to the internal message -> It's an empty String, i think we should keep the internal message super("The plasma store ran out of memory.");
If Add new constructors -> The origin two constructors are of no use.
So String message added here is just to adapt env->ThrowNew(exceptionClass, "");
There was a problem hiding this comment.
Hey @offthewall123 My concern is that someone somewhere might be using those exceptions in other ways and we shouldn't break their code. I think something like super("The plasma store ran out of memory. " + message) will result in effectively no change from current behaviour but if someone does add more context to the exception then the user will know about it.
There was a problem hiding this comment.
Hi @rymurr, i got it. Added a new constuctor without breaking the origin two.Unit test passed also.
There was a problem hiding this comment.
@rymurr Could take a review again?
| super("The plasma store ran out of memory."); | ||
| } | ||
|
|
||
| public PlasmaOutOfMemoryException(Throwable t) { | ||
| public PlasmaOutOfMemoryException(String message, Throwable t) { |
offthewall123
force-pushed
the
miss_parameter_bug_fix
branch
from
08fc7a9
to
d27d2b3
Compare
kszucs
force-pushed
the
miss_parameter_bug_fix
branch
from
f0298e9
to
c7072f7
Compare
Hi @rymurr ,will you merge this PR? |
| ByteBuffer byteBuffer = client.create(objectId, 200000000, null); | ||
| client.seal(objectId); | ||
| } catch (PlasmaOutOfMemoryException e) { | ||
| System.out.println(String.format("Expected PlasmaOutOfMemoryException: %s", e)); |
There was a problem hiding this comment.
shouldn't this have an assert?
There was a problem hiding this comment.
out of curiosity why isn't this entire class run as a unit or integration test? This currently isn't being run as part of any CI right?
There was a problem hiding this comment.
shouldn't this have an assert?
Hi @emkornfield , an Assert statement added.
|
+1, thanks. |
Hi @emkornfield Why this PR is closed ? not merged？ |
|
We use a script to merge PRs that squashes all commits and then merges the squashed commit closing the original PR. You can see the link to the new commit above |
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java Closes apache#7815 from offthewall123/miss_parameter_bug_fix Authored-by: offthewall123 <dingyu.xu@intel.com> Signed-off-by: Micah Kornfield <emkornfield@gmail.com>
Miss parameters in PlasmaOutOfMemoryException.java