Skip to content
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.

Sign up for GitHub

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-26122: Only create ScannerContext for Gets with setMaxResultSize > 0 (ADDENDUM) #3576

Closed
wants to merge 2 commits into from
Closed

HBASE-26122: Only create ScannerContext for Gets with setMaxResultSize > 0 (ADDENDUM) #3576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8caf93d
HBASE-26122: Only create ScannerContext for Gets with setMaxResultSiz…
bbeaudreault Aug 11, 2021
a236c2e
use default scanner context if null is passed
bbeaudreault Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2669,9 +2669,11 @@ private Result get(Get get, HRegion region, RegionScannersCloseCallBack closeCal
scan.setLoadColumnFamiliesOnDemand(region.isLoadingCfsOnDemandDefault());
}

ScannerContext scannerContext = ScannerContext.newBuilder()
ScannerContext scannerContext = get.getMaxResultSize() > 0
? ScannerContext.newBuilder()
.setSizeLimit(LimitScope.BETWEEN_CELLS, get.getMaxResultSize(), get.getMaxResultSize())
.build();
.build()
: null;

RegionScannerImpl scanner = null;
try {
Expand Down Expand Up @@ -2702,7 +2704,7 @@ private Result get(Get get, HRegion region, RegionScannersCloseCallBack closeCal
region.metricsUpdateForGet(results, before);

return Result.create(results, get.isCheckExistenceOnly() ? !results.isEmpty() : null, stale,
scannerContext.mayHaveMoreCellsInRow());
scannerContext != null && scannerContext.mayHaveMoreCellsInRow());
}

private void checkBatchSizeAndLogLargeSize(MultiRequest request) throws ServiceException {
Expand Down
3 changes: 3 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScannerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,9 @@ public synchronized boolean next(List<Cell> outResults, ScannerContext scannerCo
}
region.startRegionOperation(Operation.SCAN);
try {
if (scannerContext == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
}

Copy link
Contributor Author

@bbeaudreault bbeaudreault Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to submit comment from earlier this mornings... see above @bbeaudreault Thanks.

scannerContext = defaultScannerContext;
}
return nextRaw(outResults, scannerContext);
} finally {
region.closeRegionOperation(Operation.SCAN);
Expand Down