virajjasani commented on code in PR #1736: URL: https://github.com/apache/phoenix/pull/1736#discussion_r1450798009
########## phoenix-core-client/src/main/java/org/apache/phoenix/iterate/OffsetResultIterator.java: ########## @@ -32,32 +34,49 @@ */ public class OffsetResultIterator extends DelegateResultIterator { private int rowCount; - private int offset; + private final int offset; + private Tuple lastScannedTuple; private long pageSizeMs = Long.MAX_VALUE; + private boolean isIncompatibleClient = false; public OffsetResultIterator(ResultIterator delegate, Integer offset) { super(delegate); this.offset = offset == null ? -1 : offset; + this.lastScannedTuple = null; } - public OffsetResultIterator(ResultIterator delegate, Integer offset, long pageSizeMs) { + public OffsetResultIterator(ResultIterator delegate, Integer offset, long pageSizeMs, + boolean isIncompatibleClient) { this(delegate, offset); this.pageSizeMs = pageSizeMs; + this.isIncompatibleClient = isIncompatibleClient; } + @Override public Tuple next() throws SQLException { + long startTime = EnvironmentEdgeManager.currentTimeMillis(); while (rowCount < offset) { Tuple tuple = super.next(); - if (tuple == null) { return null; } + if (tuple == null) { + return null; + } if (tuple.size() == 0 || isDummy(tuple)) { // while rowCount < offset absorb the dummy and call next on the underlying scanner continue; } rowCount++; - // no page timeout check at this level because we cannot correctly resume - // scans for OFFSET queries until the offset is reached + lastScannedTuple = tuple; + if (!isIncompatibleClient) { + if (EnvironmentEdgeManager.currentTimeMillis() - startTime >= pageSizeMs) { Review Comment: We have region lock in `getOffsetScanner()` so this whole operation is under region lock. However, the reason why any lower/upper level dummy is getting ignored as of now is because `getOffsetKvWithLastScannedRowKey()` returns the last scanned rowkey and discard dummy. So in reality, it's true that we can remove this logic of timeout here, but even if we don't, `RegionScannerResultIterator` will continue `scanner#nextRaw` as dummy is discarded by `getOffsetKvWithLastScannedRowKey()`. However, now i see that we should not timeout here because even if dummy is discarded, when the offset scanner returns the result, region lock is lost and hence before `RegionScannerResultIterator` performs next `scanner#nextRaw` call, region can potentially move. Let me revisit this. -- 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: issues-unsubscr...@phoenix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org