-
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-25709 Close region may stuck when region is compacting and skip… #4536
Conversation
💔 -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. |
Ping @virajjasani , please take a look in your convenience, thanks. |
Thanks for your reply, @virajjasani . Yes, the condition in |
Ping @virajjasani, please take a look in your convenience, thanks. |
Sorry @sunhelly, could not get time. Will try my best to come back on this soon, but in the meantime if anyone else also wants to review, that is also fine. |
e622690
to
62b1840
Compare
Hi @bbeaudreault , could you take a review for this PR in your convenience? Thanks. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
// And when reaching the heartbeat cells, try to return from the loop. | ||
if ( | ||
scannerContext.checkSizeLimit(LimitScope.BETWEEN_CELLS) | ||
|| kvsScanned % cellsPerHeartbeatCheck == 0 |
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.
should this only apply to !matcher.isUserScan()
?
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.
We have discussed about the feature applying to compaction scan or user scan in https://issues.apache.org/jira/browse/HBASE-25709. Actually, we want the scans not stuck/loop in skip cells, and both the user and the compaction scans should return as soon as possible.
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.
After HBASE-27558 that you added size limit checker in the loop, it can applies to only compaction scans now.
if (scannerContext.checkSizeLimit(LimitScope.BETWEEN_CELLS)) { return scannerContext.setScannerState(NextState.MORE_VALUES).hasMoreValues(); }
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.
I wonder actually if all of this can be simplified based on my change in HBASE-27558 -- What if we just did scannerContext.setSizeLimit(ScannerContext.LimitScope.BETWEEN_CELLS, Long.MAX_VALUE, Long.MAX_VALUE, 100*1024*1024)
in Compactor? Force the StoreScanner to return every 100mb or so of blocks scanned.
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.
I think the kvsScanned % cellsPerHeartbeatCheck
was meant to avoid the cost of checking time limit (which requires hitting the clock). You're re-using it here as well to force compaction to check-in periodically based on cells/bytes scanned. But that's actually the whole point behind the changes in HBASE-27558, so that makes me think we should just apply the same change to compactions by specificying a block size limit in Compator.
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.
Seems good idea. It's reasonable to change the solution after the headway in HBASE-27558. Thanks.
...server/src/main/java/org/apache/hadoop/hbase/regionserver/querymatcher/ScanQueryMatcher.java
Outdated
Show resolved
Hide resolved
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.
I'm still confused by what the problem is and how these changes fix it. I understand it has something to do with large rows and TTL, but the changes here aren't super obvious how they help
I also am concerned about adding extra unnecessary comparisons in the hot code
The propose of this issue is to address the infinate loop in scan(compaction) caused by expired cells, as described in HBASE-25709, since the
When the |
c5f6d1a
to
ec76106
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…ped most cells read
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
…ped most cells read (apache#4536) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read (apache#4536) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read (apache#4536) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read (apache#4536) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read (#4536) (#5086) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read (#4536) (#5085) Signed-off-by: Bryan Beaudreault <[email protected]>
…ped most cells read