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 2022/07/21 14:09:25 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
lidavidm opened a new pull request, #13678:
URL: https://github.com/apache/arrow/pull/13678
- MemoryUtil will print a stack trace to the console on failure (since Arrow won't really work without it)
- Flight utility classes will print a stack trace but won't prevent class initialization (it will just fall back to the "slow" path)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930507223
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -133,7 +133,11 @@ public Object run() {
}
DIRECT_BUFFER_CONSTRUCTOR = directBufferConstructor;
} catch (Throwable e) {
- throw new RuntimeException("Failed to initialize MemoryUtil.", e);
+ // This exception will get swallowed, but it's necessary for the static analysis that ensures
+ // the static fields above get initialized
+ final RuntimeException failure = new RuntimeException("Failed to initialize MemoryUtil", e);
+ failure.printStackTrace();
Review Comment:
Interesting, in my experience only explicit logs made it to the logfiles (and java.util.logging wasn't consistently redirected)…though maybe that's not a great example
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13678:
URL: https://github.com/apache/arrow/pull/13678#issuecomment-1209125558
@lidavidm Should this be 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mystic-lama commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
mystic-lama commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930411901
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java:
##########
@@ -31,16 +31,7 @@
* Netty classes and underlying Netty memory management.
*/
public class UnsafeDirectLittleEndian extends WrappedByteBuf {
-
- public static final boolean ASSERT_ENABLED;
private static final AtomicLong ID_GENERATOR = new AtomicLong(0);
-
Review Comment:
Is this just cleanup? trying to understand it's relation with JIRA
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13678:
URL: https://github.com/apache/arrow/pull/13678#issuecomment-1191776700
https://issues.apache.org/jira/browse/ARROW-17113
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13678:
URL: https://github.com/apache/arrow/pull/13678#issuecomment-1209273314
Thanks for the reminder, this slipped under my radar
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mystic-lama commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
mystic-lama commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930411210
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -133,7 +133,11 @@ public Object run() {
}
DIRECT_BUFFER_CONSTRUCTOR = directBufferConstructor;
} catch (Throwable e) {
- throw new RuntimeException("Failed to initialize MemoryUtil.", e);
+ // This exception will get swallowed, but it's necessary for the static analysis that ensures
+ // the static fields above get initialized
+ final RuntimeException failure = new RuntimeException("Failed to initialize MemoryUtil", e);
+ failure.printStackTrace();
Review Comment:
Should we redirect the stacktrace to logger.error?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mystic-lama commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
mystic-lama commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930459986
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -133,7 +133,11 @@ public Object run() {
}
DIRECT_BUFFER_CONSTRUCTOR = directBufferConstructor;
} catch (Throwable e) {
- throw new RuntimeException("Failed to initialize MemoryUtil.", e);
+ // This exception will get swallowed, but it's necessary for the static analysis that ensures
+ // the static fields above get initialized
+ final RuntimeException failure = new RuntimeException("Failed to initialize MemoryUtil", e);
+ failure.printStackTrace();
Review Comment:
Most cases this won't be an issue since console output is also redirected to logs. Only a very small percentage who miss redirecting console output to logs shall have to look further.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #13678:
URL: https://github.com/apache/arrow/pull/13678
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930425344
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -133,7 +133,11 @@ public Object run() {
}
DIRECT_BUFFER_CONSTRUCTOR = directBufferConstructor;
} catch (Throwable e) {
- throw new RuntimeException("Failed to initialize MemoryUtil.", e);
+ // This exception will get swallowed, but it's necessary for the static analysis that ensures
+ // the static fields above get initialized
+ final RuntimeException failure = new RuntimeException("Failed to initialize MemoryUtil", e);
+ failure.printStackTrace();
Review Comment:
I wanted this to fail as loudly as possible since Arrow just won't work without it
##########
java/memory/memory-netty/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java:
##########
@@ -31,16 +31,7 @@
* Netty classes and underlying Netty memory management.
*/
public class UnsafeDirectLittleEndian extends WrappedByteBuf {
-
- public static final boolean ASSERT_ENABLED;
private static final AtomicLong ID_GENERATOR = new AtomicLong(0);
-
Review Comment:
These were unused, I was examining all static initializer blocks so I just decided to remove these
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mystic-lama commented on a diff in pull request #13678: ARROW-17113: [Java] Fail loudly in static initializer blocks
Posted by GitBox <gi...@apache.org>.
mystic-lama commented on code in PR #13678:
URL: https://github.com/apache/arrow/pull/13678#discussion_r930558959
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/MemoryUtil.java:
##########
@@ -133,7 +133,11 @@ public Object run() {
}
DIRECT_BUFFER_CONSTRUCTOR = directBufferConstructor;
} catch (Throwable e) {
- throw new RuntimeException("Failed to initialize MemoryUtil.", e);
+ // This exception will get swallowed, but it's necessary for the static analysis that ensures
+ // the static fields above get initialized
+ final RuntimeException failure = new RuntimeException("Failed to initialize MemoryUtil", e);
+ failure.printStackTrace();
Review Comment:
In my experience, we always had System error/out redirected to a log file and the logger log file is also there. Eventually all the logs make it into Splunk.
Anyways nothing major, we can always refine later. Let's plan to merge the PR once committers approve
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org