bbeaudreault commented on code in PR #4940:
URL: https://github.com/apache/hbase/pull/4940#discussion_r1065847393
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RegionScannerImpl.java:
##########
@@ -426,6 +426,8 @@ private boolean nextInternal(List<Cell> results,
ScannerContext scannerContext)
// Used to check time limit
LimitScope limitScope = LimitScope.BETWEEN_CELLS;
+ checkpoint(State.START);
Review Comment:
Thanks again @comnetwork.
I agree ideally we could do what you say. The reason I have the complexity
is because I'm concerned about use-cases of HFileScanner or StoreFileScanner
which don't go through StoreScanner. For example HFilePrettyPrinter, bulk load
verification, etc.
The `lastCheckpointIndex < 0` check ensures that we only honor
`shouldRetainBlock` if a call to `checkpoint(State)` has occurred. The contract
is that if you are using checkpointing, you also need to call `retainBlock()`.
If you are **not** using checkpointing, you don't need to call retainBlock().
Failing to call `retainBlock()` would result in blocks being released too
early, so I only want this to apply for StoreScanner, which is the only place
we currently do checkpointing.
A better approach might be to add a new `boolean checkpointEnabled` in
HFileScannerImpl constructor. This is more explicit but involves adding boolean
arguments to many various getScanner methods. I can give this a shot, or also
open to other ideas if you have them.
Let me know if this wasn't clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
