HBASE-23195 FSDataInputStreamWrapper unbuffer can NOT invoke the clas… #746
Conversation
This file contains 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
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer
jojochuang
reviewed
Oct 22, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
requested changes
Oct 23, 2019
| new FSDataInputStreamWrapper(new FSDataInputStream(pc)); | ||
| fsdisw1.unbuffer(); | ||
| // parent class should be true | ||
| assertEquals(true, fsdisw1.instanceOfCanUnbuffer); |
There was a problem hiding this comment.
Instead of checking instanceOfCanUnbuffer is set to true once called unbuffer() , you can actually assert whether the unbuffer() in pc is been called or not. Same with ChildClass case also. Can have some boolen there been set on call to unbuffer() and u can assert that here. So no need to even change the visibility of instanceOfCanUnbuffer
There was a problem hiding this comment.
@anoopsjohn Thanks for review! Good suggestion! Changed on your comments, please review it again. Thanks!
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
jojochuang
reviewed
Oct 23, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Outdated
Show resolved
Hide resolved
Apache9
requested changes
Oct 24, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
zhaoyim
commented
Oct 25, 2019
| this.unbuffer.invoke(wrappedStream); | ||
| } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { | ||
| this.unbuffer.unbuffer(); | ||
| } catch (IllegalArgumentException | UnsupportedOperationException e){ |
There was a problem hiding this comment.
I added the try catch here, because some UT failed such as TestHFile. The failed message is "this stream org.apache.hadoop.fs.BufferedFSInputStream does not support unbuffering." check the code the BufferedFSInputStream is not implements the unbuffer, through FSDataInputStream implements unbuffer but the InputStream paramenter maybe NOT, so we should catch it.
There was a problem hiding this comment.
The unbuffer here is an CanUnbuffer instance so why we could still get this exception? And for BufferedFSInputStream, it is not a sub class of CanUnbuffer? Could you please check the UT again?
There was a problem hiding this comment.
@Apache9 My fault! We do NOT need catch the IllegalAccessException. Yes, the BufferedFSInputStream is NOT a sub class of CanUnbuffer, checked in the hadoop-common project. So need to catch the UnsupportedOperationException, because the BufferedFSInputStream is NOT a sub class of CanUnbuffer. Commit a change remove catch IllegalAccessException. Thanks for your review and point out my fault.
There was a problem hiding this comment.
@Apache9 Could you help review again? Thanks!
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
LGTM +1 |
Apache9
requested changes
Oct 29, 2019
| this.unbuffer.invoke(wrappedStream); | ||
| } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { | ||
| this.unbuffer.unbuffer(); | ||
| } catch (IllegalArgumentException | UnsupportedOperationException e){ |
There was a problem hiding this comment.
The unbuffer here is an CanUnbuffer instance so why we could still get this exception? And for BufferedFSInputStream, it is not a sub class of CanUnbuffer? Could you please check the UT again?
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Oct 31, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Show resolved
Hide resolved
joshelser
requested changes
Nov 12, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Can those who requested changes ok this last PR if they agree so we can get it in? Thanks. |
|
I think Duo's request https://github.com/apache/hbase/pull/746/files#r341146952 still wasn't addressed (just use |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
1 similar comment
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
1 similar comment
|
💔 -1 overall
This message was automatically generated. |
|
@zhaoyim mind addressing the conflicts and the last comment by Duo? Thanks. |
|
@saintstack Sorry! I missed this issue. Sure! I will go through the commit then do the update based on Dou's comments This week. Thanks for reminder! |
|
@saintstack @Apache9 I updated the commit use the instanceof replaced the isAssignableFrom, please help review again, Thanks! |
|
💔 -1 overall
This message was automatically generated. |
2 similar comments
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
joshelser
requested changes
Jun 8, 2020
hbase-server/src/main/java/org/apache/hadoop/hbase/io/FSDataInputStreamWrapper.java
Show resolved
Hide resolved
|
@Apache9 shout if you have any issues, please. Otherwise, I'll come back and merge this. |
Apache9
approved these changes
Jun 12, 2020
asfgit
pushed a commit
that referenced
this pull request
Jun 12, 2020
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer Closes #746 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>
asfgit
pushed a commit
that referenced
this pull request
Jun 12, 2020
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer Closes #746 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>
asfgit
pushed a commit
that referenced
this pull request
Jun 12, 2020
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer Closes #746 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer Closes apache#746 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer Closes apache#746 Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Josh Elser <elserj@apache.org> (cherry picked from commit 0b73497) Change-Id: Ic9c953930b69bf76f3d651b69a4c10896d1502b7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…ses that NOT implements CanUnbuffer but its parents class implements CanUnbuffer