-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-26036 DBB released too early in HRegion.get() and dirty data for some operations #3436
Conversation
sunhelly
commented
Jun 29, 2021
•
edited
Loading
edited
- Add UT which can reproduce the error, and before HBASE-25187 regionserver JVM crashes.
- Fix the scanner use in checkAndMutate.
- Fix all the use of HRegion.get() except tests.
- Make HRegion.get copy cells to heap
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Sorry, I do not fully understand the problem here. Skimmed the code, the change is to use RegionScanner directly instead of the get method. Does this mean the get method is broken as it releases the ByteBuff too early? But I think we use this method everywhere in the HRegion class, we do not need to change them? Thanks. |
Hi, @Apache9 , yes the get method is broken, the other places using this method should also be changed. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits and a suggestion
hbase-common/src/main/java/org/apache/hadoop/hbase/io/ByteBuffAllocator.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MultiRowMutationEndpoint.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Show resolved
Hide resolved
currentValue = currentValues.get(currentValuesIndex); | ||
if (i < (deltas.size() - 1) && !CellUtil.matchingQualifier(delta, deltas.get(i + 1))) { | ||
currentValuesIndex++; | ||
List<Cell> currentValues = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit.... Does currentValues have to be out here? Usually you put it after you open the try {.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll change it.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Region.java
Outdated
Show resolved
Hide resolved
492cb88
to
f762abb
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
I have fixed the test issue after the commit 9f86148. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@saintstack @Apache9 @anoopsjohn PTAL, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 (again).
This is great. A nit but only if you make a new PR.
hbase-common/src/main/java/org/apache/hadoop/hbase/io/DeallocateRewriteByteBuffAllocator.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MultiRowMutationEndpoint.java
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
The failed UT is not related to this issue. |
…r some operations (apache#3436) Signed-off-by: Michael Stack <stack@apache.org>
…r some operations (apache#3436) (apache#3486) Signed-off-by: Michael Stack <stack@apache.org>