comnetwork commented on code in PR #4940:
URL: https://github.com/apache/hbase/pull/4940#discussion_r1066726624
##########
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:
@bbeaudreault , thank you for the elaborate work and I agree your point that
adding a new boolean `checkpointEnabled` make the logic more clear.
> 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().
I agree with the first part of the sentence, but have doubts about the
second. From you code, I think for scan by `RegionScannnerImpl`, `retainBlock`
is always needed to release block early, only when there is filter(especially
row level filter),we need checkpoint further to narrow the blocks which should
be retained？So we could only use `retainBlock` for user scan which
specifying columns but not has filters. If you agree with my point, I think
the variable name `boolean checkpointingEnabled` is not very appropriate， I
think maybe `earlyReleaseBlockEnabled` is more indicative of the intentions？
--
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]
