You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "sanjeet006py (via GitHub)" <gi...@apache.org> on 2026/07/16 20:56:38 UTC
[PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
sanjeet006py opened a new pull request, #2574:
URL: https://github.com/apache/phoenix/pull/2574
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://phoenix.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP] PHOENIX-XXXX Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes, and prefix it with the JIRA id, e.g. 'PHOENIX-XXXX Your PR title ...'.
6. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR makes Phoenix's internal current-row read during secondary-index maintenance TTL-mask exactly like an ordinary client read, so a
data table and its secondary index stay consistent after TTL expiry.
Index maintenance in IndexRegionObserver.preBatchMutateWithExceptions reads the current on-disk row via getCurrentRowStates to rebuild
the correct index entry. That read was opened directly through region.getScanner(scan), which bypasses the postScannerOpen coprocessor
hook — the only place a scan is normally wrapped in TTLRegionScanner. As a result the internal read was not TTL-masked, so on a "partial
touch" upsert (an UPSERT that does not re-write any index-referenced column) the index was rebuilt from
logically-expired-but-not-yet-compacted cells, while the data table correctly expired them on read.
Changes:
1. Client side — ScanUtil.annotateMutationWithLiteralTTL (called from MutationState) threads onto each mutation exactly what the client
read path sets as scan attributes: the empty-column CF/CQ (unconditionally for any literal-TTL table/view), a view's compiled literal
TTL as the _TTL attribute (base tables rely on the CF-descriptor TTL), and IS_STRICT_TTL=false only when the table/view is non-strict.
2. Server side — new ServerScanUtil (package org.apache.phoenix.coprocessor, module phoenix-core-server) reproduces the client read path
server-side: setInternalScanAttributes / setInternalScanAttributesForPaging set the same empty-column/TTL/strict/paging attributes, and
openRegionScanner wraps the scan in TTLRegionScanner + PagingRegionScanner exactly as postScannerOpen does. IndexRegionObserver
captures the client-threaded attributes into the batch context (extractLiteralTTLForInternalScan) and passes them into
getCurrentRowStates.
3. Anchor the masking clock at batchTimestamp — batchTimestamp is computed before the current-row read, and the internal scan sets
scan.setTimeRange(0, batchTimestamp) so TTLRegionScanner's "current time" equals the exact timestamp at which the index is rebuilt,
closing a sub-millisecond boundary window between the scan-open wall clock and batchTimestamp. The half-open range drops nothing
current, since every pre-existing locked-row cell was written at ts < batchTimestamp.
4. Companion correctness fix — moving getBatchTimestamp earlier means a registered set could be structurally mutated outside its
synchronized block; batchesWithLastTimestamp now registers a defensive TreeSet snapshot to avoid a ConcurrentModificationException,
staying conservative for the sleep/timestamp invariant (at most an extra sleep, never a missed one).
Production files touched: IndexRegionObserver, ScanUtil, new ServerScanUtil, MutationState, and a doc-comment update in MetaDataClient.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
This is a silent data-integrity/correctness bug. On a table or view with a literal TTL and a secondary index, after TTL expiry the
secondary index can return a column value the data table no longer returns — the two diverge with no error raised. The divergence
surfaces after logical TTL expiry and before major compaction physically purges the expired cells.
The root cause is that the internal current-row read bypassed TTLRegionScanner, so index rebuild saw expired cells the data-side read
masks. The affected paths are: secondary indexes (global covered, global uncovered, and immutable indexes whose data/index storage
schemes differ) and the no-index current-row reads on a literal-TTL table (atomic / ON DUPLICATE KEY upserts, returnResult upserts, and
row deletes). Conditional-TTL, non-TTL, and non-strict-TTL tables are unaffected — masking is a no-op there and the read is
byte-identical to before.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Phoenix versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes — a bug fix (behavior change) relative to master and released versions, for tables/views with a literal TTL and a secondary index.
- Before: on a partial-touch upsert near/after the TTL boundary, the secondary index could return a value that the data table no longer
returned (index resurrected a logically-expired value); data and index disagreed.
- After: the internal current-row read is TTL-masked identically to a client read and anchored at batchTimestamp, so the index is
rebuilt from the same masked state the data table exposes, and the two stay consistent.
No API, syntax, or configuration change. No new config flag is introduced.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
New integration test IndexDataTableSyncIT (parameterized over column-encoded on/off), covering:
- base-table covered-column re-sync;
- uncovered-index indexed-column re-sync;
- view covered-column re-sync;
- non-strict table is not masked;
- no-index atomic (ON DUPLICATE KEY) upsert masks an expired row;
- UPSERT ... SELECT and UPSERT ... VALUES touches near the TTL boundary keep data and index consistent;
- an immutable index with a differing storage scheme;
- a concurrency regression (testConcurrentMajorCompactionDuringIndexWriteKeepsDataAndIndexConsistent) where a data-table major
compaction runs during a slow index write — after the current-row read and after the data locks are released, but before the index write
completes — and data/index must remain consistent.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
Generated-by: Claude Code (Anthropic Claude Opus 4.8)
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786335785
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -794,18 +808,16 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress<Mutation> min
*/
private void updateMutationsForConditionalTTL(MiniBatchOperationInProgress<Mutation> miniBatchOp,
BatchMutateContext context) throws IOException {
- // If TTL is not strict, skip conditional TTL processing
- if (!context.isStrictTTLForInternalScan) {
+ if (!context.isConditionalTTL()) {
Review Comment:
Are we mixing the two concepts here conditional ttl and literal ttl and strict vs non-strict ttl. These are independent properties and all 4 combinations are possible and permitted.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786581237
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1770,18 +1768,10 @@ private void extractLiteralTTLForInternalScan(MiniBatchOperationInProgress<Mutat
if (ttlBytes == null) {
return;
}
- // Disambiguate literal vs conditional by the proto discriminator alone (hasLiteral()), the same
- // field TTLExpressionFactory.createFromProto keys on, without materializing the compiled
- // conditional Expression tree just to test its type. This keeps the conditional path's cost
- // unchanged: a conditional _TTL is left in place and only updateMutationsForConditionalTTL
- // deserializes it (per expiring row, as before).
- if (!PTableProtos.TTLExpression.parseFrom(ttlBytes).hasLiteral()) {
- // Conditional TTL: leave the attribute in place for updateMutationsForConditionalTTL.
- return;
- }
- context.literalTTLForInternalScan = ttlBytes;
- for (int i = 0; i < miniBatchOp.size(); i++) {
- miniBatchOp.getOperation(i).setAttribute(BaseScannerRegionObserverConstants.TTL, null);
+ CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes);
+ context.ttlExpressionForBatch = ttlExpr;
+ if (context.isLiteralTTL()) {
+ context.literalTTLForInternalScan = ttlBytes;
Review Comment:
If you want to store raw bytes I would just store the raw ttl bytes all the time and rename the variable accordingly.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3722454394
##########
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:
Had offline discussion, will unify the attributes. Thanks
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3709916965
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1228,20 +1270,41 @@ private void getCurrentRowStates(ObserverContext<RegionCoprocessorEnvironment> c
scanRanges.initializeScan(scan);
SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
scan.setFilter(skipScanFilter);
+ // Anchor TTLRegionScanner's masking clock at batchTimestamp. The range upper bound is
+ // batchTimestamp + 1 (not batchTimestamp) because HBase scan time ranges are half-open
+ // [min, max): a bound of batchTimestamp would exclude any committed cell sitting at exactly
+ // batchTimestamp. That collision cannot happen in production (getBatchTimestamp forces a
+ // distinct timestamp for same-row batches), but a frozen test clock can produce it, and the
+ // read must see those cells to build a correct current-row state. The masking clock is then
+ // batchTimestamp + 1, trimming expired cells as of the timestamp the index is built at.
+ scan.setTimeRange(0, batchTimestamp + 1);
+ ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF,
+ emptyCQ, literalTTLForScan, isStrictTTL);
readDataTableRows(c, context, scan);
}
}
private void readDataTableRows(ObserverContext<RegionCoprocessorEnvironment> c,
BatchMutateContext context, Scan scan) throws IOException {
- try (RegionScanner scanner = c.getEnvironment().getRegion().getScanner(scan)) {
+ // Open through ServerScanUtil so the scan is wrapped in TTLRegionScanner (mirroring
+ // postScannerOpen) and masks TTL-expired rows exactly like a client read. Masking no-ops when
+ // Phoenix compaction is off or the empty-column attributes are absent.
+ try (RegionScanner scanner =
+ ServerScanUtil.openRegionScanner(c.getEnvironment(), c.getEnvironment().getRegion(), scan)) {
boolean more = true;
while (more) {
List<Cell> cells = new ArrayList<Cell>();
more = scanner.next(cells);
if (cells.isEmpty()) {
continue;
}
+ // With server paging wired in (ServerScanUtil.setInternalScanAttributes),
+ // PagingRegionScanner
+ // returns a dummy result when a page is paged out; skip it and let the loop resume rather
+ // than build a Put from the dummy cell.
+ if (ScanUtil.isDummy(cells)) {
+ continue;
+ }
Review Comment:
There is no explicit functional gain rather this is scan path and my understanding is we want to cover all scan paths via paging to ensure fair utilization of server resources/handler threads. Thus, added paging explicitly. This way internal scan done by IndexRegionObserver by directly opening region scanner won't bypass paging.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707688572
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
+import org.apache.hadoop.hbase.regionserver.Region;
+import org.apache.hadoop.hbase.regionserver.RegionScanner;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants;
+import org.apache.phoenix.filter.PagingFilter;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.phoenix.schema.types.PBoolean;
+import org.apache.phoenix.util.ScanUtil;
+
+/**
+ * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client
+ * read. The client normally sets the empty-column and TTL scan attributes
+ * ({@link org.apache.phoenix.util.ScanUtil#setScanAttributesForPhoenixTTL}) and the coprocessor
+ * hook {@code BaseScannerRegionObserver.postScannerOpen} wraps the scan in a
+ * {@link TTLRegionScanner}. Internal scans opened directly via {@code region.getScanner(scan)}
+ * bypass that hook, so they set no attributes and are never TTL-masked. These helpers reproduce
+ * both steps for server-side callers (e.g. {@code IndexRegionObserver} current-row reads) so an
+ * internal scan masks identically to a client scan.
+ */
+public class ServerScanUtil {
Review Comment:
Why did we introduce a server side scanner which duplicates most of the work the existing client scanner does. An alternative option would have been to make sure these scans are also handled by BaseScannerRegionObserver by making sure `isRegionObserverFor(scan)` returns true for these
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707812489
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1228,20 +1270,41 @@ private void getCurrentRowStates(ObserverContext<RegionCoprocessorEnvironment> c
scanRanges.initializeScan(scan);
SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
scan.setFilter(skipScanFilter);
+ // Anchor TTLRegionScanner's masking clock at batchTimestamp. The range upper bound is
+ // batchTimestamp + 1 (not batchTimestamp) because HBase scan time ranges are half-open
+ // [min, max): a bound of batchTimestamp would exclude any committed cell sitting at exactly
+ // batchTimestamp. That collision cannot happen in production (getBatchTimestamp forces a
+ // distinct timestamp for same-row batches), but a frozen test clock can produce it, and the
+ // read must see those cells to build a correct current-row state. The masking clock is then
+ // batchTimestamp + 1, trimming expired cells as of the timestamp the index is built at.
+ scan.setTimeRange(0, batchTimestamp + 1);
+ ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF,
+ emptyCQ, literalTTLForScan, isStrictTTL);
readDataTableRows(c, context, scan);
}
}
private void readDataTableRows(ObserverContext<RegionCoprocessorEnvironment> c,
BatchMutateContext context, Scan scan) throws IOException {
- try (RegionScanner scanner = c.getEnvironment().getRegion().getScanner(scan)) {
+ // Open through ServerScanUtil so the scan is wrapped in TTLRegionScanner (mirroring
+ // postScannerOpen) and masks TTL-expired rows exactly like a client read. Masking no-ops when
+ // Phoenix compaction is off or the empty-column attributes are absent.
+ try (RegionScanner scanner =
+ ServerScanUtil.openRegionScanner(c.getEnvironment(), c.getEnvironment().getRegion(), scan)) {
boolean more = true;
while (more) {
List<Cell> cells = new ArrayList<Cell>();
more = scanner.next(cells);
if (cells.isEmpty()) {
continue;
}
+ // With server paging wired in (ServerScanUtil.setInternalScanAttributes),
+ // PagingRegionScanner
+ // returns a dummy result when a page is paged out; skip it and let the loop resume rather
+ // than build a Put from the dummy cell.
+ if (ScanUtil.isDummy(cells)) {
+ continue;
+ }
Review Comment:
I am not sure what are we gaining with paging here ?
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707859516
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
+import org.apache.hadoop.hbase.regionserver.Region;
+import org.apache.hadoop.hbase.regionserver.RegionScanner;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants;
+import org.apache.phoenix.filter.PagingFilter;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.phoenix.schema.types.PBoolean;
+import org.apache.phoenix.util.ScanUtil;
+
+/**
+ * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client
+ * read. The client normally sets the empty-column and TTL scan attributes
+ * ({@link org.apache.phoenix.util.ScanUtil#setScanAttributesForPhoenixTTL}) and the coprocessor
+ * hook {@code BaseScannerRegionObserver.postScannerOpen} wraps the scan in a
+ * {@link TTLRegionScanner}. Internal scans opened directly via {@code region.getScanner(scan)}
+ * bypass that hook, so they set no attributes and are never TTL-masked. These helpers reproduce
+ * both steps for server-side callers (e.g. {@code IndexRegionObserver} current-row reads) so an
+ * internal scan masks identically to a client scan.
+ */
+public class ServerScanUtil {
+
+ private ServerScanUtil() {
+ }
+
+ /**
+ * Sets the Phoenix TTL and paging scan attributes on an internal data-table scan so it behaves
+ * exactly like a client read.
+ * <p>
+ * TTL masking attributes ({@link TTLRegionScanner} reads these):
+ * <ul>
+ * <li>the empty-column CF/CQ, supplied by the caller from the bytes the client threaded on the
+ * mutation ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) — the single
+ * source for every path, secondary-index and no-index (atomic / ON DUPLICATE KEY /
+ * {@code returnResult} / row-delete) alike;</li>
+ * <li>{@code IS_STRICT_TTL=false} when {@code isStrictTTL == false}, so a non-strict table is not
+ * masked (absence of the attribute defaults to strict, matching the read path);</li>
+ * <li>the view's literal TTL as the standard {@code _TTL} scan attribute when
+ * {@code literalTTLForScan != null}. A base table's literal TTL is left unset so
+ * {@link TTLRegionScanner}'s CF-descriptor fallback derives it.</li>
+ * </ul>
+ */
+ public static void setInternalScanAttributes(Configuration conf, Scan scan, byte[] emptyCF,
+ byte[] emptyCQ, byte[] literalTTLForScan, boolean isStrictTTL) {
+ scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, emptyCF);
+ scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, emptyCQ);
+ if (!isStrictTTL) {
+ // Absence of the attribute defaults to strict-true (ScanUtil.isStrictTTL), so only set it
+ // when the table/view is non-strict, mirroring setScanAttributesForPhoenixTTL.
+ scan.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL,
+ PBoolean.INSTANCE.toBytes(false));
+ }
+ if (literalTTLForScan != null) {
+ // 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.
+ * <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)));
+ }
+ }
Review Comment:
This whole code has been duplicated from the client but there seems to be little benefit since we are doing a region local scan which doesn't go through the rpc path so what is paging benefit we are trying to achieve ?
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786490451
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1770,18 +1768,10 @@ private void extractLiteralTTLForInternalScan(MiniBatchOperationInProgress<Mutat
if (ttlBytes == null) {
return;
}
- // Disambiguate literal vs conditional by the proto discriminator alone (hasLiteral()), the same
- // field TTLExpressionFactory.createFromProto keys on, without materializing the compiled
- // conditional Expression tree just to test its type. This keeps the conditional path's cost
- // unchanged: a conditional _TTL is left in place and only updateMutationsForConditionalTTL
- // deserializes it (per expiring row, as before).
- if (!PTableProtos.TTLExpression.parseFrom(ttlBytes).hasLiteral()) {
- // Conditional TTL: leave the attribute in place for updateMutationsForConditionalTTL.
- return;
- }
- context.literalTTLForInternalScan = ttlBytes;
- for (int i = 0; i < miniBatchOp.size(); i++) {
- miniBatchOp.getOperation(i).setAttribute(BaseScannerRegionObserverConstants.TTL, null);
+ CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes);
+ context.ttlExpressionForBatch = ttlExpr;
+ if (context.isLiteralTTL()) {
+ context.literalTTLForInternalScan = ttlBytes;
Review Comment:
Ideally no. But instead of reserializiing TTL Expression (which we have already extracted) for literal TTL, I thought why store that also as bytes. Literal TTL in bytes form doesn't add much overhead.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3720151184
##########
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 intentionally used a new mutation attribute for literal TTL as currently in `updateMutationsForConditionalTTL` there is a blind cast of TTL expression to Conditional TTL expression and moreover, in IndexRegionObserver there is implicit assumption that if `_TTL` attribute is set on a mutation then its conditional TTL. If client changes in this PR gets deployed earlier then server changes, then that can break mutation path by through ClassCastException.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786551839
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -794,18 +808,16 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress<Mutation> min
*/
private void updateMutationsForConditionalTTL(MiniBatchOperationInProgress<Mutation> miniBatchOp,
BatchMutateContext context) throws IOException {
- // If TTL is not strict, skip conditional TTL processing
- if (!context.isStrictTTLForInternalScan) {
+ if (!context.isConditionalTTL()) {
Review Comment:
If you think this can cause confusion then I can add the check for strictTTL back. This was a nit change so, I am fine either way.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#issuecomment-5318521574
@sanjeet006py Thanks for making the changes. This looks neat and clean now.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#issuecomment-5090138890
PR validation: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-2574/
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3721817026
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1528,6 +1528,10 @@ private void sendMutations(Iterator<Entry<TableInfo, List<Mutation>>> mutationsI
// no-op if table doesn't have Conditional TTL
ScanUtil.annotateMutationWithConditionalTTL(connection, tableInfo.getPTable(),
mutationList);
+ // no-op unless table/view has a literal TTL; threads the empty-column CF/CQ (plus a view's
+ // literal TTL and any non-strict flag) so the internal current-row scan masks like a client
+ // read
+ ScanUtil.annotateMutationWithLiteralTTL(connection, tableInfo.getPTable(), mutationList);
Review Comment:
Its ok. There are already existing tests.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3749675549
##########
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:
@tkhurana I have addressed the suggestion. PTAL
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707688572
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
+import org.apache.hadoop.hbase.regionserver.Region;
+import org.apache.hadoop.hbase.regionserver.RegionScanner;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants;
+import org.apache.phoenix.filter.PagingFilter;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.phoenix.schema.types.PBoolean;
+import org.apache.phoenix.util.ScanUtil;
+
+/**
+ * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client
+ * read. The client normally sets the empty-column and TTL scan attributes
+ * ({@link org.apache.phoenix.util.ScanUtil#setScanAttributesForPhoenixTTL}) and the coprocessor
+ * hook {@code BaseScannerRegionObserver.postScannerOpen} wraps the scan in a
+ * {@link TTLRegionScanner}. Internal scans opened directly via {@code region.getScanner(scan)}
+ * bypass that hook, so they set no attributes and are never TTL-masked. These helpers reproduce
+ * both steps for server-side callers (e.g. {@code IndexRegionObserver} current-row reads) so an
+ * internal scan masks identically to a client scan.
+ */
+public class ServerScanUtil {
Review Comment:
Why did we introduce a server side scanner which duplicates most of the work the existing client scanner does. An alternative option would have been to make sure these scans are also handled by BaseScannerRegionObserver by making sure `isRegionObserverFor(scan)` returns true for these
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786533624
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -794,18 +808,16 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress<Mutation> min
*/
private void updateMutationsForConditionalTTL(MiniBatchOperationInProgress<Mutation> miniBatchOp,
BatchMutateContext context) throws IOException {
- // If TTL is not strict, skip conditional TTL processing
- if (!context.isStrictTTLForInternalScan) {
+ if (!context.isConditionalTTL()) {
Review Comment:
The invocation of method `updateMutationsForConditionalTTL` is gated by pre-existing `hasConditionalTTL` in `preBatchMutateWithExceptions`. `hasConditionalTTL` is true when mutation batch has conditionalTTL along with strict TTL being true. Given conditional TTL and strict TTL are two independent properties then gating by the caller of the method makes sense so, I removed the check from method itself.
But separately added a check for conditionalTTL explicitly as this method should never be called when its not conditional TTL. Its more of defense in depth check as whole method is relevant for conditional TTL only.
So, I am not mixing two concepts.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786585161
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -794,18 +808,16 @@ private void addOnDupMutationsToBatch(MiniBatchOperationInProgress<Mutation> min
*/
private void updateMutationsForConditionalTTL(MiniBatchOperationInProgress<Mutation> miniBatchOp,
BatchMutateContext context) throws IOException {
- // If TTL is not strict, skip conditional TTL processing
- if (!context.isStrictTTLForInternalScan) {
+ if (!context.isConditionalTTL()) {
Review Comment:
There is confusion. We have `hasConditionalTTL` which is true when when mutation batch has conditionalTTL along with strict TTL being true. That is confusing naming wise. We also have `context.hasConditionalTTL` and `context.isConditionalTTL`.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py merged PR #2574:
URL: https://github.com/apache/phoenix/pull/2574
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3721859683
##########
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:
The current check was done because it was a cheap way to do it but the server can deserialize the ttl expression and can easily determine if the expression is a literal ttl or conditional ttl. If the server is upgraded first which is typically the case it will remain backward compatible to any client using conditional ttl. I am not sure if we really need to worry about the case of the client getting deployed before server. That can cause all sorts of other issues.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3720151184
##########
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 intentionally used a new mutation attribute for literal TTL as currently in `updateMutationsForConditionalTTL` there is a blind cast of TTL expression to Conditional TTL expression and moreover, in IndexRegionObserver there is implicit assumption that if `_TTL` attribute is set on a mutation then its conditional TTL. If client changes in this PR gets deployed earlier then server changes, then that can break mutation path by throwing ClassCastException.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3709541576
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1528,6 +1528,10 @@ private void sendMutations(Iterator<Entry<TableInfo, List<Mutation>>> mutationsI
// no-op if table doesn't have Conditional TTL
ScanUtil.annotateMutationWithConditionalTTL(connection, tableInfo.getPTable(),
mutationList);
+ // no-op unless table/view has a literal TTL; threads the empty-column CF/CQ (plus a view's
+ // literal TTL and any non-strict flag) so the internal current-row scan masks like a client
+ // read
+ ScanUtil.annotateMutationWithLiteralTTL(connection, tableInfo.getPTable(), mutationList);
Review Comment:
I observed that conditional TTL case was already covered by `IndexRegionObserver#updateMutationsForConditionalTTL`. There we check if a row is already expired and add delete markers.
I didn't explicitly verify via IT. Do you think I should add one?
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3709947315
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1228,20 +1270,41 @@ private void getCurrentRowStates(ObserverContext<RegionCoprocessorEnvironment> c
scanRanges.initializeScan(scan);
SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
scan.setFilter(skipScanFilter);
+ // Anchor TTLRegionScanner's masking clock at batchTimestamp. The range upper bound is
+ // batchTimestamp + 1 (not batchTimestamp) because HBase scan time ranges are half-open
+ // [min, max): a bound of batchTimestamp would exclude any committed cell sitting at exactly
+ // batchTimestamp. That collision cannot happen in production (getBatchTimestamp forces a
+ // distinct timestamp for same-row batches), but a frozen test clock can produce it, and the
+ // read must see those cells to build a correct current-row state. The masking clock is then
+ // batchTimestamp + 1, trimming expired cells as of the timestamp the index is built at.
+ scan.setTimeRange(0, batchTimestamp + 1);
+ ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF,
+ emptyCQ, literalTTLForScan, isStrictTTL);
readDataTableRows(c, context, scan);
}
}
private void readDataTableRows(ObserverContext<RegionCoprocessorEnvironment> c,
BatchMutateContext context, Scan scan) throws IOException {
- try (RegionScanner scanner = c.getEnvironment().getRegion().getScanner(scan)) {
+ // Open through ServerScanUtil so the scan is wrapped in TTLRegionScanner (mirroring
+ // postScannerOpen) and masks TTL-expired rows exactly like a client read. Masking no-ops when
+ // Phoenix compaction is off or the empty-column attributes are absent.
+ try (RegionScanner scanner =
+ ServerScanUtil.openRegionScanner(c.getEnvironment(), c.getEnvironment().getRegion(), scan)) {
boolean more = true;
while (more) {
List<Cell> cells = new ArrayList<Cell>();
more = scanner.next(cells);
if (cells.isEmpty()) {
continue;
}
+ // With server paging wired in (ServerScanUtil.setInternalScanAttributes),
+ // PagingRegionScanner
+ // returns a dummy result when a page is paged out; skip it and let the loop resume rather
+ // than build a Put from the dummy cell.
+ if (ScanUtil.isDummy(cells)) {
+ continue;
+ }
Review Comment:
Oh, I didn't relaize that if its not going through RPC path then it won't even consume additional handler threads. Yeah, there is no benefit of paging in here. Thanks, will remove it.
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1228,20 +1270,41 @@ private void getCurrentRowStates(ObserverContext<RegionCoprocessorEnvironment> c
scanRanges.initializeScan(scan);
SkipScanFilter skipScanFilter = scanRanges.getSkipScanFilter();
scan.setFilter(skipScanFilter);
+ // Anchor TTLRegionScanner's masking clock at batchTimestamp. The range upper bound is
+ // batchTimestamp + 1 (not batchTimestamp) because HBase scan time ranges are half-open
+ // [min, max): a bound of batchTimestamp would exclude any committed cell sitting at exactly
+ // batchTimestamp. That collision cannot happen in production (getBatchTimestamp forces a
+ // distinct timestamp for same-row batches), but a frozen test clock can produce it, and the
+ // read must see those cells to build a correct current-row state. The masking clock is then
+ // batchTimestamp + 1, trimming expired cells as of the timestamp the index is built at.
+ scan.setTimeRange(0, batchTimestamp + 1);
+ ServerScanUtil.setInternalScanAttributes(c.getEnvironment().getConfiguration(), scan, emptyCF,
+ emptyCQ, literalTTLForScan, isStrictTTL);
readDataTableRows(c, context, scan);
}
}
private void readDataTableRows(ObserverContext<RegionCoprocessorEnvironment> c,
BatchMutateContext context, Scan scan) throws IOException {
- try (RegionScanner scanner = c.getEnvironment().getRegion().getScanner(scan)) {
+ // Open through ServerScanUtil so the scan is wrapped in TTLRegionScanner (mirroring
+ // postScannerOpen) and masks TTL-expired rows exactly like a client read. Masking no-ops when
+ // Phoenix compaction is off or the empty-column attributes are absent.
+ try (RegionScanner scanner =
+ ServerScanUtil.openRegionScanner(c.getEnvironment(), c.getEnvironment().getRegion(), scan)) {
boolean more = true;
while (more) {
List<Cell> cells = new ArrayList<Cell>();
more = scanner.next(cells);
if (cells.isEmpty()) {
continue;
}
+ // With server paging wired in (ServerScanUtil.setInternalScanAttributes),
+ // PagingRegionScanner
+ // returns a dummy result when a page is paged out; skip it and let the loop resume rather
+ // than build a Put from the dummy cell.
+ if (ScanUtil.isDummy(cells)) {
+ continue;
+ }
Review Comment:
Oh, I didn't realize that if its not going through RPC path then it won't even consume additional handler threads. Yeah, there is no benefit of paging in here. Thanks, will remove it.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707704134
##########
phoenix-core-client/src/main/java/org/apache/phoenix/execute/MutationState.java:
##########
@@ -1528,6 +1528,10 @@ private void sendMutations(Iterator<Entry<TableInfo, List<Mutation>>> mutationsI
// no-op if table doesn't have Conditional TTL
ScanUtil.annotateMutationWithConditionalTTL(connection, tableInfo.getPTable(),
mutationList);
+ // no-op unless table/view has a literal TTL; threads the empty-column CF/CQ (plus a view's
+ // literal TTL and any non-strict flag) so the internal current-row scan masks like a client
+ // read
+ ScanUtil.annotateMutationWithLiteralTTL(connection, tableInfo.getPTable(), mutationList);
Review Comment:
Did you verify that conditional ttl doesn't have this problem ? I didn't see any test cases with conditional ttl
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3710104411
##########
phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/ServerScanUtil.java:
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.phoenix.coprocessor;
+
+import java.io.IOException;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.coprocessor.RegionCoprocessorEnvironment;
+import org.apache.hadoop.hbase.regionserver.Region;
+import org.apache.hadoop.hbase.regionserver.RegionScanner;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants;
+import org.apache.phoenix.filter.PagingFilter;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.query.QueryServicesOptions;
+import org.apache.phoenix.schema.types.PBoolean;
+import org.apache.phoenix.util.ScanUtil;
+
+/**
+ * Utilities for internal server-side region scans that must honor Phoenix TTL exactly like a client
+ * read. The client normally sets the empty-column and TTL scan attributes
+ * ({@link org.apache.phoenix.util.ScanUtil#setScanAttributesForPhoenixTTL}) and the coprocessor
+ * hook {@code BaseScannerRegionObserver.postScannerOpen} wraps the scan in a
+ * {@link TTLRegionScanner}. Internal scans opened directly via {@code region.getScanner(scan)}
+ * bypass that hook, so they set no attributes and are never TTL-masked. These helpers reproduce
+ * both steps for server-side callers (e.g. {@code IndexRegionObserver} current-row reads) so an
+ * internal scan masks identically to a client scan.
+ */
+public class ServerScanUtil {
+
+ private ServerScanUtil() {
+ }
+
+ /**
+ * Sets the Phoenix TTL and paging scan attributes on an internal data-table scan so it behaves
+ * exactly like a client read.
+ * <p>
+ * TTL masking attributes ({@link TTLRegionScanner} reads these):
+ * <ul>
+ * <li>the empty-column CF/CQ, supplied by the caller from the bytes the client threaded on the
+ * mutation ({@link org.apache.phoenix.util.ScanUtil#annotateMutationWithLiteralTTL}) — the single
+ * source for every path, secondary-index and no-index (atomic / ON DUPLICATE KEY /
+ * {@code returnResult} / row-delete) alike;</li>
+ * <li>{@code IS_STRICT_TTL=false} when {@code isStrictTTL == false}, so a non-strict table is not
+ * masked (absence of the attribute defaults to strict, matching the read path);</li>
+ * <li>the view's literal TTL as the standard {@code _TTL} scan attribute when
+ * {@code literalTTLForScan != null}. A base table's literal TTL is left unset so
+ * {@link TTLRegionScanner}'s CF-descriptor fallback derives it.</li>
+ * </ul>
+ */
+ public static void setInternalScanAttributes(Configuration conf, Scan scan, byte[] emptyCF,
+ byte[] emptyCQ, byte[] literalTTLForScan, boolean isStrictTTL) {
+ scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_FAMILY_NAME, emptyCF);
+ scan.setAttribute(BaseScannerRegionObserverConstants.EMPTY_COLUMN_QUALIFIER_NAME, emptyCQ);
+ if (!isStrictTTL) {
+ // Absence of the attribute defaults to strict-true (ScanUtil.isStrictTTL), so only set it
+ // when the table/view is non-strict, mirroring setScanAttributesForPhoenixTTL.
+ scan.setAttribute(BaseScannerRegionObserverConstants.IS_STRICT_TTL,
+ PBoolean.INSTANCE.toBytes(false));
+ }
+ if (literalTTLForScan != null) {
+ // 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.
+ * <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)));
+ }
+ }
Review Comment:
Thanks for highlighting this explicitly that no handler threads will be involved here.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3707750443
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1854,7 +1967,8 @@ public void preBatchMutateWithExceptions(ObserverContext<RegionCoprocessorEnviro
|| !context.immutableRows && context.hasUncoveredIndex
&& isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp)
) {
- getCurrentRowStates(c, context);
+ getCurrentRowStates(c, context, context.literalTTLForInternalScan,
+ isStrictTTLEnabled(miniBatchOp), batchTimestamp);
Review Comment:
We can save is strict ttl in also in the context. You are already saving ttl and empty column family and qualifier. That will make the API cleaner.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "sanjeet006py (via GitHub)" <gi...@apache.org>.
sanjeet006py commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786490451
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1770,18 +1768,10 @@ private void extractLiteralTTLForInternalScan(MiniBatchOperationInProgress<Mutat
if (ttlBytes == null) {
return;
}
- // Disambiguate literal vs conditional by the proto discriminator alone (hasLiteral()), the same
- // field TTLExpressionFactory.createFromProto keys on, without materializing the compiled
- // conditional Expression tree just to test its type. This keeps the conditional path's cost
- // unchanged: a conditional _TTL is left in place and only updateMutationsForConditionalTTL
- // deserializes it (per expiring row, as before).
- if (!PTableProtos.TTLExpression.parseFrom(ttlBytes).hasLiteral()) {
- // Conditional TTL: leave the attribute in place for updateMutationsForConditionalTTL.
- return;
- }
- context.literalTTLForInternalScan = ttlBytes;
- for (int i = 0; i < miniBatchOp.size(); i++) {
- miniBatchOp.getOperation(i).setAttribute(BaseScannerRegionObserverConstants.TTL, null);
+ CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes);
+ context.ttlExpressionForBatch = ttlExpr;
+ if (context.isLiteralTTL()) {
+ context.literalTTLForInternalScan = ttlBytes;
Review Comment:
Ideally no. But instead of reserializiing TTL Expression (which we have already extracted) for literal TTL, I thought why not store that also as bytes. Literal TTL in bytes form doesn't add much overhead.
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PHOENIX-7961 Secondary index diverges from data table after TTL expiry on partial-touch upserts [phoenix]
Posted by "tkhurana (via GitHub)" <gi...@apache.org>.
tkhurana commented on code in PR #2574:
URL: https://github.com/apache/phoenix/pull/2574#discussion_r3786323393
##########
phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java:
##########
@@ -1770,18 +1768,10 @@ private void extractLiteralTTLForInternalScan(MiniBatchOperationInProgress<Mutat
if (ttlBytes == null) {
return;
}
- // Disambiguate literal vs conditional by the proto discriminator alone (hasLiteral()), the same
- // field TTLExpressionFactory.createFromProto keys on, without materializing the compiled
- // conditional Expression tree just to test its type. This keeps the conditional path's cost
- // unchanged: a conditional _TTL is left in place and only updateMutationsForConditionalTTL
- // deserializes it (per expiring row, as before).
- if (!PTableProtos.TTLExpression.parseFrom(ttlBytes).hasLiteral()) {
- // Conditional TTL: leave the attribute in place for updateMutationsForConditionalTTL.
- return;
- }
- context.literalTTLForInternalScan = ttlBytes;
- for (int i = 0; i < miniBatchOp.size(); i++) {
- miniBatchOp.getOperation(i).setAttribute(BaseScannerRegionObserverConstants.TTL, null);
+ CompiledTTLExpression ttlExpr = TTLExpressionFactory.create(ttlBytes);
+ context.ttlExpressionForBatch = ttlExpr;
+ if (context.isLiteralTTL()) {
+ context.literalTTLForInternalScan = ttlBytes;
Review Comment:
Do we need a separate field for literal ttl ?
--
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-unsubscribe@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org