tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3714649289
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java:
##########
@@ -78,57 +76,21 @@ public static void setInternalScanAttributes(Configuration
conf, Scan scan, byte
// Only views carry a literal TTL here; a base table relies on the
CF-descriptor fallback.
scan.setAttribute(BaseScannerRegionObserverConstants.TTL,
literalTTLForScan);
}
- setInternalScanAttributesForPaging(conf, scan);
}
/**
- * Reproduces the client read path's server-paging setup for an internal
scan. On the client the
- * {@code SERVER_PAGE_SIZE_MS} attribute is set by
- * {@code ScanUtil.setScanAttributeForPaging(Scan, PhoenixConnection)} and
the scan filter is
- * later wrapped in a {@link PagingFilter} by {@code
BaseScannerRegionObserver.preScannerOpen}.
- * Internal scans opened directly via {@code region.getScanner(scan)} bypass
both, so this method
- * performs both steps up-front. The region-server {@link Configuration} is
the source of the
- * paging props here, standing in for the client's {@code PhoenixConnection}
props.
+ * Opens a region scanner wrapped in a {@link TTLRegionScanner} exactly as
+ * {@code BaseScannerRegionObserver.postScannerOpen} wraps a client scan, so
TTL masking is
+ * applied. This is always safe: {@link TTLRegionScanner#isMaskingEnabled}
no-ops the masking when
+ * Phoenix compaction is disabled, the empty-column attributes are absent,
the TTL is FOREVER, or
+ * the scan is non-strict — so wrapping a non-TTL scan changes no behavior.
* <p>
- * Ordering matters: {@code PagingRegionScanner}'s constructor reads the
{@link PagingFilter} and
- * the page size off the scan, so this must run before
- * {@link #openRegionScanner(RegionCoprocessorEnvironment, Region, Scan)}
builds the scanner.
- */
- public static void setInternalScanAttributesForPaging(Configuration conf,
Scan scan) {
- if (
- !conf.getBoolean(QueryServices.PHOENIX_SERVER_PAGING_ENABLED_ATTRIB,
- QueryServicesOptions.DEFAULT_PHOENIX_SERVER_PAGING_ENABLED)
- ) {
- return;
- }
- long pageSizeMs = conf.getInt(QueryServices.PHOENIX_SERVER_PAGE_SIZE_MS,
-1);
- if (pageSizeMs == -1) {
- // Use half of the HBase RPC timeout value as the server page size,
mirroring the client
- // ScanUtil.setScanAttributeForPaging fallback.
- pageSizeMs =
- (long) (conf.getLong(HConstants.HBASE_RPC_TIMEOUT_KEY,
HConstants.DEFAULT_HBASE_RPC_TIMEOUT)
- * 0.5);
- }
- scan.setAttribute(BaseScannerRegionObserverConstants.SERVER_PAGE_SIZE_MS,
- Bytes.toBytes(Long.valueOf(pageSizeMs)));
- // Wrap the scan filter in a PagingFilter as the top-level filter, matching
- // BaseScannerRegionObserver.preScannerOpen. PagingRegionScanner then
detects when PagingFilter
- // has paged the scan out and returns a dummy result; readDataTableRows
skips those dummies.
- if (!(scan.getFilter() instanceof PagingFilter)) {
- scan.setFilter(new PagingFilter(scan.getFilter(),
ScanUtil.getPageSizeMsForFilter(scan)));
- }
- }
-
- /**
- * Opens a region scanner wrapped exactly as {@code
BaseScannerRegionObserver.postScannerOpen}
- * wraps a client scan, so TTL masking is applied. This is always safe:
- * {@link TTLRegionScanner#isMaskingEnabled} no-ops the masking when Phoenix
compaction is
- * disabled, the empty-column attributes are absent, the TTL is FOREVER, or
the scan is non-strict
- * — so wrapping a non-TTL scan changes no behavior.
+ * Unlike the client read path this does not wrap in a {@code
PagingRegionScanner}: the scan is
+ * region-local (opened directly on the {@link Region}, off the RPC path),
so it holds no handler
+ * thread and has nothing for paging to protect.
*/
public static RegionScanner openRegionScanner(RegionCoprocessorEnvironment
env, Region region,
Scan scan) throws IOException {
- return new TTLRegionScanner(env, scan,
- new PagingRegionScanner(region, region.getScanner(scan), scan));
+ return new TTLRegionScanner(env, scan, region.getScanner(scan));
Review Comment:
It might not be safe in all cases to directly pass the hbase regionscanner
as the delegate to TTLRegionScanner. There are code paths where we cast to
DelegateRegionScanner.
##########
phoenix-core-client/src/main/java/org/apache/phoenix/coprocessorclient/BaseScannerRegionObserverConstants.java:
##########
@@ -131,6 +131,12 @@ public static long getMaxLookbackInMillis(Configuration
conf) {
public static final String SKIP_REGION_BOUNDARY_CHECK =
"_SKIP_REGION_BOUNDARY_CHECK";
public static final String TX_SCN = "_TxScn";
public static final String TTL = "_TTL";
+ // Literal TTL threaded per-mutation for the server-side internal
current-row scan
Review Comment:
I think it is better to use the same TTL attribute for both conditional and
literal ttl. We already do that on the scan path. There is no reason we can't
do that on the mutation path also.
--
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]
