HBASE-26122: Only create ScannerContext for Gets with setMaxResultSize > 0 (ADDENDUM) #3576
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
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 11, 2021
| @@ -240,6 +240,9 @@ public synchronized boolean next(List<Cell> outResults, ScannerContext scannerCo | |||
| } | |||
| region.startRegionOperation(Operation.SCAN); | |||
| try { | |||
| if (scannerContext == null) { | |||
There was a problem hiding this comment.
@saintstack if you'd prefer, I can solve this at the source where we call next with null scannerContext in RsRpcServices and HRegion. It felt cleaner to do here, since we already have a default context and I can solve both issues with 1 change. The alternative would be to add another if branch in the 2 callers, something like:
if (scannerContext == null) {
scanner.next(results);
} else {
scanner.next(results, scannerContext);
}There was a problem hiding this comment.
It's also worth noting that this means the original concern "every get will now have a scanner context, when it used to not" actually seems incorrect since each RegionScannerImpl would create a default scanner context if none is provided. I should have noticed this before straight up fixing your suggestion.
I don't know enough about the inner workings of ScannerContext to know the performance implications of one context shape (the default) vs another (with max result size). If there's no real difference, than perhaps the real change here is to just go back to creating a ScannerContext for every get like I originally did.
Will wait for your thoughts
There was a problem hiding this comment.
After your finding, it seems like your original PR is the way to go. Thanks for figuring it.
I reverted the branch-2 apply. One thought if you are making a new PR is for the javadoc on setMaxResultSize; you say, 'The user is required to handle this case.'... Perhaps add example of what they might do... i.e. a re-get w/ filter say?
Thanks @bbeaudreault
There was a problem hiding this comment.
I guess the remaining implication of this would be we are now creating 2 ScannerContext for every Get instead of 1. So I'll just keep the behavior of only creating a ScannerContext if necessary.
Closing this PR and submitting a follow-up, since the original was reverted.
There was a problem hiding this comment.
I forgot to submit comment from earlier this mornings... see above @bbeaudreault Thanks.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Closing as original was reverted. |
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.
@saintstack After your suggestion in #3532 I had meant to also apply the change to RSRpcServices, but got pulled away and didn't submit before you merged.
This small patch applies that same change to RSRpcServices. I'll include this in the master branch PR