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 with 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 blocks early, only when there is filter(especially
row level filter),we need checkpoint further to narrow the blocks which should
be retained after a row is filtered？I think we could just only use
`retainBlock` for user scan which specifying columns but not has filters, so
we don't need to call `checkpoint`. If you agree with my point, I think the
variable name `boolean checkpointingEnabled` is not very appropriate, maybe
`earlyReleaseBlockEnabled` is more indicative of 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]
