You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/21 10:57:18 UTC
[GitHub] [arrow] offthewall123 opened a new pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
offthewall123 opened a new pull request #7815:
URL: https://github.com/apache/arrow/pull/7815
Miss parameters in PlasmaOutOfMemoryException.java
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r462686721
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review comment:
@wesm
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
emkornfield commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r466162890
##########
File path: java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java
##########
@@ -277,6 +278,20 @@ public void doByteBufferTest() {
client.release(id);
}
+ public void doPlasmaOutOfMemoryExceptionTest() {
+ System.out.println("Start PlasmaOutOfMemoryException test.");
+ PlasmaClient client = (PlasmaClient) pLink;
+ byte[] objectId = new byte[20];
+ Arrays.fill(objectId, (byte) 1);
+ try {
+ ByteBuffer byteBuffer = client.create(objectId, 200000000, null);
+ client.seal(objectId);
+ } catch (PlasmaOutOfMemoryException e) {
+ System.out.println(String.format("Expected PlasmaOutOfMemoryException: %s", e));
Review comment:
shouldn't this have an assert?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-670812386
+1, thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r459381715
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r458679661
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
super("The plasma store ran out of memory.");
}
- public PlasmaOutOfMemoryException(Throwable t) {
+ public PlasmaOutOfMemoryException(String message, Throwable t) {
Review comment:
Same here
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review comment:
Shouldn't we pass the string somewhere? eg add it to the original message?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r459825776
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review 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, "");`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r461977379
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review comment:
@rymurr Could take a review again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r459170893
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-669629818
> LGTM
Hi @rymurr ,will you merge this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-662208991
@wesm Could you take a review?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r466964416
##########
File path: java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java
##########
@@ -277,6 +278,20 @@ public void doByteBufferTest() {
client.release(id);
}
+ public void doPlasmaOutOfMemoryExceptionTest() {
+ System.out.println("Start PlasmaOutOfMemoryException test.");
+ PlasmaClient client = (PlasmaClient) pLink;
+ byte[] objectId = new byte[20];
+ Arrays.fill(objectId, (byte) 1);
+ try {
+ ByteBuffer byteBuffer = client.create(objectId, 200000000, null);
+ client.seal(objectId);
+ } catch (PlasmaOutOfMemoryException e) {
+ System.out.println(String.format("Expected PlasmaOutOfMemoryException: %s", e));
Review 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
emkornfield commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-670940242
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-661789403
https://issues.apache.org/jira/browse/ARROW-9536
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r460602206
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review comment:
Hi @rymurr, i got it. Added a new constuctor without breaking the origin two.Unit test passed also.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#issuecomment-670933488
> +1, thanks.
Hi @emkornfield Why this PR is closed ? not merged？
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r467342682
##########
File path: java/plasma/src/test/java/org/apache/arrow/plasma/PlasmaClientTest.java
##########
@@ -277,6 +278,20 @@ public void doByteBufferTest() {
client.release(id);
}
+ public void doPlasmaOutOfMemoryExceptionTest() {
+ System.out.println("Start PlasmaOutOfMemoryException test.");
+ PlasmaClient client = (PlasmaClient) pLink;
+ byte[] objectId = new byte[20];
+ Arrays.fill(objectId, (byte) 1);
+ try {
+ ByteBuffer byteBuffer = client.create(objectId, 200000000, null);
+ client.seal(objectId);
+ } catch (PlasmaOutOfMemoryException e) {
+ System.out.println(String.format("Expected PlasmaOutOfMemoryException: %s", e));
Review comment:
> shouldn't this have an assert?
Hi @emkornfield , an Assert statement added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] emkornfield closed pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
emkornfield closed pull request #7815:
URL: https://github.com/apache/arrow/pull/7815
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] offthewall123 commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
offthewall123 commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r459188625
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review comment:
An unit test added, please take a review.
@rymurr
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] rymurr commented on a change in pull request #7815: ARROW-9536: [Java] Miss parameters in PlasmaOutOfMemoryException.java
Posted by GitBox <gi...@apache.org>.
rymurr commented on a change in pull request #7815:
URL: https://github.com/apache/arrow/pull/7815#discussion_r459996684
##########
File path: java/plasma/src/main/java/org/apache/arrow/plasma/exceptions/PlasmaOutOfMemoryException.java
##########
@@ -22,11 +22,11 @@
*/
public class PlasmaOutOfMemoryException extends RuntimeException {
- public PlasmaOutOfMemoryException() {
+ public PlasmaOutOfMemoryException(String message) {
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org