HDFS-14348: Fix JNI exception handling issues in libhdfs#600
Closed
sahilTakiar wants to merge 4 commits intoapache:trunkfrom
Closed
HDFS-14348: Fix JNI exception handling issues in libhdfs#600sahilTakiar wants to merge 4 commits intoapache:trunkfrom
sahilTakiar wants to merge 4 commits intoapache:trunkfrom
Conversation
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Fixing cc warnings. |
|
💔 -1 overall
This message was automatically generated. |
toddlipcon
reviewed
Mar 19, 2019
|
|
||
| if (opts->byteBufferPool) { | ||
| // Delete any previous ByteBufferPool we had. | ||
| (*env)->DeleteGlobalRef(env, opts->byteBufferPool); |
Contributor
There was a problem hiding this comment.
This should probably be moved down until right before setting the new value. Otherwise, if we failed somewhere below, we'd end up with two issues: (1) the function would return an error but had a side effect of clearing the previous value, which seems unintuitive. (2) the function would still leave opts->byteBufferPool set, but with a now-invalid reference.
Contributor
Author
There was a problem hiding this comment.
Done. Fixed a few other issues I found with this method while I was at it.
| jFileBlock = | ||
| (*env)->GetObjectArrayElement(env, jBlockLocations, i); | ||
| if (!jFileBlock) { | ||
| if ((*env)->ExceptionOccurred || !jFileBlock) { |
Contributor
There was a problem hiding this comment.
this should be ExceptionOccurred(env), right? it's a function, not a variable, isn't it?
(same below a few places)
Contributor
Author
There was a problem hiding this comment.
Yeah, it definitely should be. Fixed.
|
💔 -1 overall
This message was automatically generated. |
* Fixed several issues in hadoopRzOptionsSetByteBufferPool * Fixed usage of ExceptionOccurred
|
💔 -1 overall
This message was automatically generated. |
toddlipcon
reviewed
Mar 21, 2019
| } else if (opts->byteBufferPool) { | ||
| // If the specified className is NULL, delete any previous | ||
| // ByteBufferPool we had. | ||
| (*env)->DeleteGlobalRef(env, opts->byteBufferPool); |
Contributor
There was a problem hiding this comment.
need to also set it back to null, right?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
toddlipcon
approved these changes
Mar 22, 2019
smengcl
pushed a commit
to smengcl/hadoop
that referenced
this pull request
Oct 8, 2019
This closes apache#600 Signed-off-by: Todd Lipcon <todd@apache.org> (cherry picked from commit fe29b39) Change-Id: I89af28e6006cfbde16897435150b1ae2d88c1c68 (cherry picked from commit dfd5401)
shanthoosh
pushed a commit
to shanthoosh/hadoop
that referenced
this pull request
Oct 15, 2019
We should enable host affinity when RocksDB table is present, this should be done in RocksDB table provider. Author: Wei Song <wsong@linkedin.com> Reviewers: Prateek Maheshwari <pmaheshwari@linkedin.com> Closes apache#600 from weisong44/add-host-affinity and squashes the following commits: 78e1b84a [Wei Song] Enable host affinity in RocksDB table provider a15a7c9 [Wei Song] Merge remote-tracking branch 'upstream/master' 5cbf9af9 [Wei Song] Merge remote-tracking branch 'upstream/master' 3f7ed71 [Wei Song] Added self to committer list
basic02
pushed a commit
to basic02/hadoop
that referenced
this pull request
Jan 12, 2025
This closes apache#600 Signed-off-by: Todd Lipcon <todd@apache.org> (cherry picked from commit fe29b39) Change-Id: I89af28e6006cfbde16897435150b1ae2d88c1c68
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HDFS-14348 contains a list of all the JNI issues this patch fixes. All of the changes are related to proper exception handling when using the JNI library.
Some highlights:
hadoopRzOptionsSetByteBufferPoolwas changed to add proper exception handling forNewGlobalRefand to cleanup some of the logic that was missed in HDFS-14321get_current_thread_idwas re-written to use methods fromjni_helper.hrather than using the JNI library directly; this fixes several issues with exception handling that were present in the code