GH-20379: [Java] Dataset Failed to update reservation while freeing bytes #40101
Conversation
|
|
| @@ -83,6 +83,40 @@ void ThrowIfError(const arrow::Status& status) { | |||
| } | |||
| } | |||
|
|
|||
| class JNIEnvGuard { | |||
| public: | |||
| explicit JNIEnvGuard(JavaVM* vm) : vm_(vm), should_detach_(false) { | |||
There was a problem hiding this comment.
This should initialize in the order the fields are declared, and env_ should be initialized. Probably better to re-order the fields such that the bool is the last field though to minimize padding.
There was a problem hiding this comment.
env_ is initialized at the end of the constructor...
There was a problem hiding this comment.
The compiler emits a build warning if it isn't done in the initializer list (even though you are setting it later).
There was a problem hiding this comment.
Ok fixed, I didn't get (or I missed) the c++ compiler's warnings with archery ...
github-actions
bot
added
awaiting committer review
| return arrow::Status::Invalid("JNIEnv was not attached to current thread"); | ||
| } | ||
| JNIEnvGuard guard(vm_); | ||
| JNIEnv* env = guard.env(); |
There was a problem hiding this comment.
This function wasn't throwing previously. It was returning an error status previously. Are the callers ready to handle exceptions?
There was a problem hiding this comment.
try/catch added to maintain the initial behavior
fb64
force-pushed
the
jni-thread
branch
2 times, most recently
from
a5486c5
to
c2132ce
Compare
…eing bytes: JNIEnv was not attached to current thread
github-actions
bot
added
awaiting merge
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit df9e0c1. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
|
@github-actions crossbow submit java-jars |
|
Revision: 9a76007 Submitted crossbow builds: ursacomputing/crossbow @ actions-09542ccf6f
|
…eing bytes (apache#40101) ### Rationale for this change Better controls JNI Thread management in java dataset module to fix apache#20379 Re-use the same code found in the java arrow-c-data module : https://github.com/apache/arrow/blob/main/java/c/src/main/cpp/jni_wrapper.cc#L107 May JNIEnvGuard class code can be put in a common place for both libraries ... ### What changes are included in this PR? N/A ### Are these changes tested? These changes has been tested with : https://gist.github.com/fb64/71880cde297bc5234b02b68b785670fd on Linux X86_64 architecture ### Are there any user-facing changes? N/A * Closes: apache#20379 Authored-by: Florian Bernard <[email protected]> Signed-off-by: David Li <[email protected]>
…eing bytes (apache#40101) ### Rationale for this change Better controls JNI Thread management in java dataset module to fix apache#20379 Re-use the same code found in the java arrow-c-data module : https://github.com/apache/arrow/blob/main/java/c/src/main/cpp/jni_wrapper.cc#L107 May JNIEnvGuard class code can be put in a common place for both libraries ... ### What changes are included in this PR? N/A ### Are these changes tested? These changes has been tested with : https://gist.github.com/fb64/71880cde297bc5234b02b68b785670fd on Linux X86_64 architecture ### Are there any user-facing changes? N/A * Closes: apache#20379 Authored-by: Florian Bernard <[email protected]> Signed-off-by: David Li <[email protected]>
Rationale for this change
Better controls JNI Thread management in java dataset module to fix #20379
Re-use the same code found in the java arrow-c-data module : https://github.com/apache/arrow/blob/main/java/c/src/main/cpp/jni_wrapper.cc#L107
May JNIEnvGuard class code can be put in a common place for both libraries ...
What changes are included in this PR?
N/A
Are these changes tested?
These changes has been tested with : https://gist.github.com/fb64/71880cde297bc5234b02b68b785670fd
on Linux X86_64 architecture
Are there any user-facing changes?
N/A