You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@phoenix.apache.org by tk...@apache.org on 2026/09/16 23:30:24 UTC
(phoenix) branch PHOENIX-7562-feature-new updated (339025a5ef -> 94fed195f5)
This is an automated email from the ASF dual-hosted git repository.
tkhurana pushed a change to branch PHOENIX-7562-feature-new
in repository https://gitbox.apache.org/repos/asf/phoenix.git
from 339025a5ef PHOENIX-7996 Enable server-side maintenance of immutable indexes by default (#2618)
new 10d06ad2f6 PHOENIX-7874 Addendum Trim redundant per-batch metric work on the indexed write path
new 94fed195f5 PHOENIX-7874 Addendum Skip redundant getMaxTimestamp scans on the active index write path
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../index/metrics/MetricsIndexerSourceImpl.java | 21 ++-
.../phoenix/hbase/index/IndexRegionObserver.java | 186 +++++++++++----------
.../phoenix/replication/MutationCellGrouper.java | 30 +++-
.../phoenix/replication/ReplicationLogGroupIT.java | 20 ++-
.../reader/ReplicationLogProcessorTestIT.java | 12 +-
5 files changed, 153 insertions(+), 116 deletions(-)
(phoenix) 02/02: PHOENIX-7874 Addendum Skip redundant getMaxTimestamp scans on the active index write path
Posted by tk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tkhurana pushed a commit to branch PHOENIX-7562-feature-new
in repository https://gitbox.apache.org/repos/asf/phoenix.git
commit 94fed195f56f30abe076a42b5b773d3eecdb5988
Author: tkhurana <kh...@gmail.com>
AuthorDate: Tue Sep 15 14:20:36 2026 -0700
PHOENIX-7874 Addendum Skip redundant getMaxTimestamp scans on the active index write path
On the active path every generated index-mutation cell carries the batch-wide
timestamp (set by setTimestamps), so re-scanning each mutation's cells with the
O(cells) IndexUtil.getMaxTimestamp on every index-prepare step was wasted work.
Introduce indexMutationTimestamp(context, m): use context.batchTimestamp on the
active path, fall back to getMaxTimestamp only on the standby replay path (which
regenerates index updates per (row, ts) group, where the mutation's own timestamp
is authoritative).
Make context.batchTimestamp the single source of truth for the index-prepare
chain: set it once where it is computed and drop the now-redundant batchTimestamp
parameter threaded through prepareAndCommitGlobalIndexUpdates,
preparePreIndexMutations, and prepareIndexMutations. Also drop the never-used
timestamp parameter from prepareDataRowStates/applyPendingPutMutations.
Standby path is unchanged (byte-for-byte). Validated by GlobalIndexCheckerIT,
GlobalIndexCheckerEventualGenerateIT, UncoveredGlobalMutableNonTxIndexIT, and
ReplicationLogGroupIT.
---
.../phoenix/hbase/index/IndexRegionObserver.java | 162 ++++++++++-----------
.../phoenix/replication/MutationCellGrouper.java | 30 +++-
.../phoenix/replication/ReplicationLogGroupIT.java | 20 ++-
.../reader/ReplicationLogProcessorTestIT.java | 12 +-
4 files changed, 122 insertions(+), 102 deletions(-)
diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
index dd887cc73f..3cc6310196 100644
--- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
+++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
@@ -134,7 +134,6 @@ import org.apache.phoenix.index.IndexMaintainer;
import org.apache.phoenix.index.PhoenixIndexBuilderHelper;
import org.apache.phoenix.index.PhoenixIndexCodec;
import org.apache.phoenix.index.PhoenixIndexMetaData;
-import org.apache.phoenix.index.PhoenixIndexMetaDataBuilder;
import org.apache.phoenix.jdbc.HAGroupStoreManager;
import org.apache.phoenix.jdbc.HAGroupStoreRecord;
import org.apache.phoenix.jdbc.HighAvailabilityPolicy;
@@ -442,6 +441,11 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
// The collection of candidate index mutations that will be applied after the data table
// mutations.
private ListMultimap<HTableInterfaceReference, Pair<Mutation, byte[]>> indexUpdates;
+ // The batch-wide timestamp stamped on every data/index mutation on the active path (see
+ // setTimestamps). Lets the index-prepare steps timestamp index mutations without re-scanning
+ // each mutation's cells. Unused on the standby replay path, which derives per-(row, ts)
+ // timestamps from each mutation directly.
+ private long batchTimestamp;
// Map of (data table row key, group ts) to IndexMutations bytes containing pre-index mutations
// for eventually consistent indexes (UNVERIFIED Puts only, no Deletes). Keyed by (row, ts) so
// multiple per-row entries from the standby's per-(row, ts) grouping don't collide. On the
@@ -1053,20 +1057,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
if (replicable.isEmpty()) {
return;
}
- Map<String, byte[]> replicationAttrs = new HashMap<>();
- for (String attrKey : ReplicationLogGroup.REPLICATION_ATTR_KEYS) {
- byte[] val = walKeyAttrs.get(attrKey);
- if (val != null) {
- replicationAttrs.put(attrKey, val);
- }
- }
// INDEX_UUID rides the WAL key only when appendReplicationAttributesToWALKey stamped it (i.e.
- // the batch's table was indexed, gated on hasIndex()); copy it through verbatim so this path
- // follows the same server-PTable resolution the synchronous path triggers.
- byte[] indexUuid = walKeyAttrs.get(PhoenixIndexCodec.INDEX_UUID);
- if (indexUuid != null) {
- replicationAttrs.put(PhoenixIndexCodec.INDEX_UUID, indexUuid);
- }
+ // the batch's table was indexed, gated on hasIndex()); its presence drives the same empty-UUID
+ // stamp here, so this path follows the same server-PTable resolution the synchronous path does.
+ // The stamped value is always empty, so re-stamping empty is equivalent to copying it through.
+ Map<String, byte[]> replicationAttrs = MutationCellGrouper.buildReplicationAttributes(
+ walKeyAttrs, walKeyAttrs.containsKey(PhoenixIndexCodec.INDEX_UUID));
logGroup.append(tableName, -1, replicable, replicationAttrs);
logGroup.sync();
}
@@ -1428,7 +1424,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
* called, the next row states is set to current row states.
*/
private void applyPendingPutMutations(MiniBatchOperationInProgress<Mutation> miniBatchOp,
- BatchMutateContext context, long now) throws IOException {
+ BatchMutateContext context) throws IOException {
for (Integer i = 0; i < miniBatchOp.size(); i++) {
if (isAtomicOperationComplete(miniBatchOp.getOperationStatus(i))) {
continue;
@@ -1467,12 +1463,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
* * Prepares next data row state
*/
private void prepareDataRowStates(ObserverContext<RegionCoprocessorEnvironment> c,
- MiniBatchOperationInProgress<Mutation> miniBatchOp, BatchMutateContext context, long now)
+ MiniBatchOperationInProgress<Mutation> miniBatchOp, BatchMutateContext context)
throws IOException {
if (context.rowsToLock.size() == 0) {
return;
}
- applyPendingPutMutations(miniBatchOp, context, now);
+ applyPendingPutMutations(miniBatchOp, context);
applyPendingDeleteMutations(miniBatchOp, context);
}
@@ -1733,13 +1729,17 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
}
/**
- * True when this batch will be shipped to the replication log: replication is on, not disabled
- * for testing, and an HA log group is present. Gates the active-side pre-image capture — the
- * pre-image exists only so the standby can regenerate its index, so capturing it on a
- * non-replicated batch would be wasted work (and an unnecessary region scan on the local path).
+ * True when this batch is replicated on an active, replication-configured cluster: replication is
+ * on and an HA log group is present. Gates the active-side pre-image capture — the pre-image
+ * exists only so the standby can regenerate its index, so capturing it on a non-replicated batch
+ * would be wasted work (and an unnecessary region scan on the local path) — and the WAL-key
+ * replication stamp. It intentionally does NOT gate on {@code ignoreSyncReplicationForTesting}:
+ * that flag suppresses only the synchronous ship (checked explicitly at the ship site) to
+ * simulate an RS crash after the WAL write but before the ship, leaving pre-image capture, the
+ * WAL stamp, and the replay path intact so crash-recovery re-ship is faithfully exercised.
*/
private boolean isReplicatedBatch(BatchMutateContext context) {
- return shouldReplicate && !ignoreSyncReplicationForTesting && context.logGroup.isPresent();
+ return shouldReplicate && context.logGroup.isPresent();
}
/**
@@ -2122,8 +2122,8 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
* Generate the index update for a data row from the mutation that are obtained by merging the
* previous data row state with the pending row mutation.
*/
- private void prepareIndexMutations(BatchMutateContext context, List<IndexMaintainer> maintainers,
- long batchTimestamp) throws IOException {
+ private void prepareIndexMutations(BatchMutateContext context, List<IndexMaintainer> maintainers)
+ throws IOException {
List<Pair<IndexMaintainer, HTableInterfaceReference>> indexTables =
buildIndexTablesList(maintainers);
for (Map.Entry<ImmutableBytesPtr, Pair<Put, Put>> entry : context.dataRowStates.entrySet()) {
@@ -2135,8 +2135,9 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
continue;
}
ListMultimap<HTableInterfaceReference, Mutation> idxUpdates = ArrayListMultimap.create();
- generateIndexMutationsForRow(rowKeyPtr, currentDataRowState, nextDataRowState, batchTimestamp,
- encodedRegionName, QueryConstants.UNVERIFIED_BYTES, indexTables, idxUpdates);
+ generateIndexMutationsForRow(rowKeyPtr, currentDataRowState, nextDataRowState,
+ context.batchTimestamp, encodedRegionName, QueryConstants.UNVERIFIED_BYTES, indexTables,
+ idxUpdates);
for (Map.Entry<HTableInterfaceReference, Mutation> idxUpdate : idxUpdates.entries()) {
context.indexUpdates.put(idxUpdate.getKey(),
new Pair<>(idxUpdate.getValue(), rowKeyPtr.get()));
@@ -2174,8 +2175,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
* row is deleted.
*/
private void preparePreIndexMutations(MiniBatchOperationInProgress<Mutation> miniBatchOp,
- BatchMutateContext context, long batchTimestamp, PhoenixIndexMetaData indexMetaData)
- throws Throwable {
+ BatchMutateContext context, PhoenixIndexMetaData indexMetaData) throws Throwable {
List<IndexMaintainer> maintainers = indexMetaData.getIndexMaintainers();
// get the current span, or just use a null-span to avoid a bunch of if statements
try (TraceScope scope = Trace.startSpan("Starting to build index updates")) {
@@ -2193,7 +2193,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
// pre-image — recovers the active-batch boundary the reader's coalescing can erase.
prepareReplicatedIndexMutations(miniBatchOp, context, maintainers);
} else {
- prepareIndexMutations(context, maintainers, batchTimestamp);
+ prepareIndexMutations(context, maintainers);
}
if (serializeCDCMutations) {
@@ -2217,7 +2217,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
List<Pair<Mutation, byte[]>> updates = context.indexUpdates.get(hTableInterfaceReference);
for (Pair<Mutation, byte[]> update : updates) {
Mutation m = update.getFirst();
- long ts = IndexUtil.getMaxTimestamp(m);
+ long ts = indexMutationTimestamp(context, m);
RowTsKey cdcKey = new RowTsKey(new ImmutableBytesPtr(update.getSecond()), ts);
if (m instanceof Put) {
if (indexMaintainer.isCDCIndex() && context.cdcPreMutationsBytes != null) {
@@ -2285,7 +2285,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
for (Pair<Mutation, byte[]> update : updates) {
Mutation m = update.getFirst();
byte[] dataRowKey = update.getSecond();
- long ts = IndexUtil.getMaxTimestamp(m);
+ long ts = indexMutationTimestamp(context, m);
RowTsKey key = new RowTsKey(new ImmutableBytesPtr(dataRowKey), ts);
IndexMutationsProtos.IndexMutations.Builder preBuilder =
preBuilderMap.computeIfAbsent(key, k -> IndexMutationsProtos.IndexMutations.newBuilder());
@@ -2373,6 +2373,17 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
return (PhoenixIndexMetaData) indexMetaData;
}
+ /**
+ * Timestamp to stamp/key a generated index mutation by. On the active path every index-mutation
+ * cell carries {@code batchTimestamp} (see {@link #setTimestamps}), so we use that directly and
+ * skip the O(cells) {@link IndexUtil#getMaxTimestamp} scan. The standby regenerates index updates
+ * per {@code (row, ts)} group, where the mutation's own timestamp is authoritative, so it must
+ * read it from the mutation.
+ */
+ private static long indexMutationTimestamp(BatchMutateContext context, Mutation m) {
+ return context.isReplication ? IndexUtil.getMaxTimestamp(m) : context.batchTimestamp;
+ }
+
private void preparePostIndexMutations(BatchMutateContext context,
PhoenixIndexMetaData indexMetaData) {
context.postIndexUpdates = ArrayListMultimap.<HTableInterfaceReference, Mutation> create();
@@ -2395,7 +2406,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
if (!indexMaintainer.isUncovered()) {
Put verifiedPut = new Put(m.getRow());
// Set the status of the index row to "verified"
- verifiedPut.addColumn(emptyCF, emptyCQ, IndexUtil.getMaxTimestamp(m),
+ verifiedPut.addColumn(emptyCF, emptyCQ, indexMutationTimestamp(context, m),
QueryConstants.VERIFIED_BYTES);
context.postIndexUpdates.put(hTableInterfaceReference, verifiedPut);
}
@@ -2416,7 +2427,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
for (Pair<Mutation, byte[]> update : updates) {
Mutation m = update.getFirst();
if (m instanceof Put) {
- long ts = IndexUtil.getMaxTimestamp(m);
+ long ts = indexMutationTimestamp(context, m);
RowTsKey cdcKey = new RowTsKey(new ImmutableBytesPtr(update.getSecond()), ts);
byte[] cdcMutationsBytes = context.cdcPostMutationsBytes.get(cdcKey);
if (cdcMutationsBytes != null) {
@@ -2707,21 +2718,20 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
}
TableName table = c.getEnvironment().getRegion().getRegionInfo().getTable();
- long batchTimestamp = getBatchTimestamp(context, table);
+ context.batchTimestamp = getBatchTimestamp(context, table);
// Update the timestamps of the data table mutations to prevent overlapping timestamps
// (which prevents index inconsistencies as this case is not handled).
- setTimestamps(miniBatchOp, builder, batchTimestamp, isStrictTTLEnabled(miniBatchOp));
+ setTimestamps(miniBatchOp, builder, context.batchTimestamp, isStrictTTLEnabled(miniBatchOp));
if (context.hasGlobalIndex || context.hasUncoveredIndex || context.hasTransform) {
// Prepare next data rows states for pending mutations (for global indexes).
- prepareDataRowStates(c, miniBatchOp, context, batchTimestamp);
+ prepareDataRowStates(c, miniBatchOp, context);
// dataRowStates is now populated; on a replicated batch write per-row pre-image cells to the
// WAL edit so both replication paths (replicateMutations and replicateEditOnWALRestore) ship
// them. Skip on a non-replicated batch — the pre-image would be unused work.
if (isReplicatedBatch(context)) {
capturePreImageCells(miniBatchOp, context);
}
- prepareAndCommitGlobalIndexUpdates(table, miniBatchOp, context, batchTimestamp,
- indexMetaData);
+ prepareAndCommitGlobalIndexUpdates(table, miniBatchOp, context, indexMetaData);
}
if (context.hasLocalIndex) {
// Group all the updates for a single row into a single update to be processed (for local
@@ -2763,9 +2773,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
PhoenixIndexMetaData indexMetaData) throws Throwable {
TableName table = c.getEnvironment().getRegion().getRegionInfo().getTable();
if (context.hasGlobalIndex || context.hasUncoveredIndex || context.hasTransform) {
- // batchTimestamp is unused on this path: prepareReplicatedIndexMutations derives each index
- // update's timestamp from its (row, ts) group, not from a batch-wide timestamp.
- prepareAndCommitGlobalIndexUpdates(table, miniBatchOp, context, 0, indexMetaData);
+ prepareAndCommitGlobalIndexUpdates(table, miniBatchOp, context, indexMetaData);
}
if (context.hasLocalIndex) {
// Group by (row, ts) so each replayed active-side batch's cells stay in their own uniform-ts
@@ -2792,10 +2800,10 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
*/
private void prepareAndCommitGlobalIndexUpdates(TableName table,
MiniBatchOperationInProgress<Mutation> miniBatchOp, BatchMutateContext context,
- long batchTimestamp, PhoenixIndexMetaData indexMetaData) throws Throwable {
+ PhoenixIndexMetaData indexMetaData) throws Throwable {
// early exit if it turns out we don't have any edits
long start = EnvironmentEdgeManager.currentTimeMillis();
- preparePreIndexMutations(miniBatchOp, context, batchTimestamp, indexMetaData);
+ preparePreIndexMutations(miniBatchOp, context, indexMetaData);
metricSource.updateIndexPrepareTime(dataTableName,
EnvironmentEdgeManager.currentTimeMillis() - start);
// Release the locks before making RPC calls for index updates
@@ -2862,16 +2870,11 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
@Override
public void preWALAppend(ObserverContext<RegionCoprocessorEnvironment> c, WALKey key,
WALEdit edit) {
+ BatchMutateContext context = getBatchMutateContext(c);
if (shouldWALAppend) {
- BatchMutateContext context = getBatchMutateContext(c);
appendMutationAttributesToWALKey(key, context);
}
-
- if (shouldReplicate) {
- BatchMutateContext context = getBatchMutateContext(c);
- appendHAGroupAttributeToWALKey(key, context);
- appendReplicationAttributesToWALKey(key, context);
- }
+ appendReplicationAttributesToWALKey(key, context);
}
private void appendMutationAttributesToWALKey(WALKey key,
@@ -2890,29 +2893,37 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
}
/**
- * Save the HA group name if present in the WAL key so that we can use it when restoring from the
- * WAL
+ * Stamp the WAL key with everything the WAL-restore re-ship path consumes: the HA group name it
+ * keys on ({@link #getHAGroupFromWALKey}) plus the replication attribute envelope
+ * (schema/table/tenant, and an empty INDEX_UUID for indexed tables) it reads back in
+ * {@link #replicateEditOnWALRestore}. Gated on {@link #isReplicatedBatch} — the same predicate
+ * that gates pre-image capture — so the WAL carries this stamp exactly when it carries the
+ * pre-images the re-ship needs. On a standby replay the log group is absent, so the standby's WAL
+ * never carries replication metadata it could never re-ship.
*/
- private void appendHAGroupAttributeToWALKey(WALKey key,
- IndexRegionObserver.BatchMutateContext context) {
- if (context != null && context.logGroup.isPresent()) {
- String haGroupName = context.logGroup.get().getHAGroupName();
- IndexRegionObserver.appendToWALKey(key,
- BaseScannerRegionObserverConstants.HA_GROUP_NAME_ATTRIB, Bytes.toBytes(haGroupName));
- }
- }
-
private void appendReplicationAttributesToWALKey(WALKey key,
IndexRegionObserver.BatchMutateContext context) {
- if (context == null || context.getOriginalMutations().isEmpty()) {
+ if (context == null || !isReplicatedBatch(context)) {
return;
}
- Map<String, byte[]> replicationAttributes = buildReplicationAttributes(context);
- for (Map.Entry<String, byte[]> e : replicationAttributes.entrySet()) {
+ IndexRegionObserver.appendToWALKey(key, BaseScannerRegionObserverConstants.HA_GROUP_NAME_ATTRIB,
+ Bytes.toBytes(context.logGroup.get().getHAGroupName()));
+ for (Map.Entry<String, byte[]> e : buildReplicationAttributes(context).entrySet()) {
IndexRegionObserver.appendToWALKey(key, e.getKey(), e.getValue());
}
}
+ /**
+ * Active-path replication envelope for a batch: the well-known metadata keys carried on the
+ * batch's first mutation, plus an empty INDEX_UUID when the table is indexed
+ * ({@link BatchMutateContext#hasIndex()}). Shared by the synchronous ship and the WAL-append
+ * stamp; the WAL-restore path builds the same envelope from the WAL key attributes instead.
+ */
+ private static Map<String, byte[]> buildReplicationAttributes(BatchMutateContext context) {
+ return MutationCellGrouper.buildReplicationAttributes(
+ context.getOriginalMutations().get(0).getAttributesMap(), context.hasIndex());
+ }
+
/**
* When this hook is called, all the rows in the batch context are locked if the batch of
* mutations is successful. Because the rows are locked, we can safely make updates to pending row
@@ -2975,7 +2986,9 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
// updates
CompletableFuture<Void> postIndexFuture =
CompletableFuture.runAsync(() -> doPost(c, context));
- if (isReplicatedBatch(context)) {
+ // ignoreSyncReplicationForTesting suppresses only this synchronous ship, simulating an RS
+ // crash after the WAL write but before the ship; the WAL-restore path re-ships on replay.
+ if (isReplicatedBatch(context) && !ignoreSyncReplicationForTesting) {
replicateMutations(context.logGroup.get(), miniBatchOp, context);
}
FutureUtils.get(postIndexFuture);
@@ -3612,23 +3625,4 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
}
}
- /**
- * Build the replication attribute envelope shipped with a batch: the well-known metadata keys
- * carried on the batch's mutations, plus an empty {@link PhoenixIndexCodec#INDEX_UUID} when (and
- * only when) the table carries an index. An empty UUID forces the standby down the server-PTable
- * resolution path (see {@link PhoenixIndexMetaDataBuilder}), which rebuilds index maintainers
- * from the schema/table/tenant attributes in this same envelope. It is stamped only for indexed
- * tables: a non-indexed table needs no regeneration, and an empty UUID there would push the
- * standby into the server-cache branch and fail with INDEX_METADATA_NOT_FOUND. The active's own
- * resolved index maintainers ({@link BatchMutateContext#hasIndex()}) are the source of truth, not
- * the client-set UUID attribute.
- */
- private static Map<String, byte[]> buildReplicationAttributes(BatchMutateContext context) {
- Map<String, byte[]> replicationAttributes =
- MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0));
- if (context.hasIndex()) {
- MutationCellGrouper.stampIndexAttribute(replicationAttributes);
- }
- return replicationAttributes;
- }
}
diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/MutationCellGrouper.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/MutationCellGrouper.java
index 210e32e63d..08413811e3 100644
--- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/MutationCellGrouper.java
+++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/MutationCellGrouper.java
@@ -81,17 +81,31 @@ public final class MutationCellGrouper {
* for indexed tables (see {@code IndexRegionObserver}).
*/
public static Map<String, byte[]> extractReplicationAttributes(Mutation mutation) {
+ return buildReplicationAttributes(mutation.getAttributesMap(), false);
+ }
+
+ /**
+ * Build the replication attribute envelope from a raw attribute source: the
+ * {@link ReplicationLogGroup#REPLICATION_ATTR_KEYS} present in {@code source}, plus an empty
+ * {@link PhoenixIndexCodec#INDEX_UUID} when {@code indexed}. Shared by the active write path
+ * (source = the batch's first mutation attributes, {@code indexed} = context.hasIndex()) and the
+ * WAL-restore path (source = the WAL key attributes, {@code indexed} = INDEX_UUID already rode
+ * the key). Returns a mutable, possibly empty map.
+ */
+ public static Map<String, byte[]> buildReplicationAttributes(Map<String, byte[]> source,
+ boolean indexed) {
Map<String, byte[]> envelope = new HashMap<>();
- Map<String, byte[]> mutationAttrs = mutation.getAttributesMap();
- if (mutationAttrs == null || mutationAttrs.isEmpty()) {
- return envelope;
- }
- for (String key : ReplicationLogGroup.REPLICATION_ATTR_KEYS) {
- byte[] v = mutationAttrs.get(key);
- if (v != null) {
- envelope.put(key, v);
+ if (source != null) {
+ for (String key : ReplicationLogGroup.REPLICATION_ATTR_KEYS) {
+ byte[] v = source.get(key);
+ if (v != null) {
+ envelope.put(key, v);
+ }
}
}
+ if (indexed) {
+ stampIndexAttribute(envelope);
+ }
return envelope;
}
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java
index f926fd70c6..4b5bd6c1d3 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupIT.java
@@ -873,14 +873,16 @@ public class ReplicationLogGroupIT extends ReplicationLogGroupBaseIT {
final String tableName = "T_" + generateUniqueName();
final String indexName = "I_" + generateUniqueName();
TableName table = TableName.valueOf(tableName);
- try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager
- .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) {
- String ddl = String.format("create table %s (id1 integer not null, "
+ final String createTableDdl =
+ String.format("create table if not exists %s (id1 integer not null, "
+ "id2 integer not null, val1 varchar, val2 varchar "
+ "constraint pk primary key (id1, id2))", tableName);
- conn.createStatement().execute(ddl);
- ddl = String.format("create index %s on %s (val1) include (val2)", indexName, tableName);
- conn.createStatement().execute(ddl);
+ final String createIndexDdl = String
+ .format("create index if not exists %s on %s (val1) include (val2)", indexName, tableName);
+ try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager
+ .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) {
+ conn.createStatement().execute(createTableDdl);
+ conn.createStatement().execute(createIndexDdl);
conn.commit();
}
// Mini cluster by default comes with only 1 RS. Starting a second RS so that
@@ -933,6 +935,12 @@ public class ReplicationLogGroupIT extends ReplicationLogGroupBaseIT {
// expected.put(SYSTEM_CATALOG_NAME, 1);
verifyReplication(expected);
}
+ // Beyond the log-entry count, replay the re-shipped log onto cluster 2 and assert the data
+ // table and the standby-regenerated index are byte-for-byte equal across clusters. This
+ // exercises index regeneration from the pre-images the WAL-restore re-ship carries -- which
+ // ignoreSyncReplicationForTesting must leave intact (it suppresses only the synchronous ship).
+ replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndexDdl), tableName,
+ indexName);
}
@Ignore("Mutations on SYSTEM.CATALOG and SYSTEM.CHILD_LINK are generated on the server side and don't have the HAGroup attribute set")
diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/reader/ReplicationLogProcessorTestIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/reader/ReplicationLogProcessorTestIT.java
index 6168d2692b..184717d961 100644
--- a/phoenix-core/src/it/java/org/apache/phoenix/replication/reader/ReplicationLogProcessorTestIT.java
+++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/reader/ReplicationLogProcessorTestIT.java
@@ -631,11 +631,14 @@ public class ReplicationLogProcessorTestIT extends ParallelStatsDisabledIT {
// Recording-site guard (PHOENIX-7992): incrementSuccessfulFileMutationsReplayedCount is
// written inside processLogFile before the catch, not by its callers, so a real replay is the
// only thing that exercises it. The exact replayed-mutation count is intentionally not
- // asserted: generateHBaseMutations builds its Puts from a Phoenix mutation state that is never
- // committed or cleared between rows, so the replayed total is an artifact of that helper rather
+ // asserted: generateHBaseMutations builds its Puts from a Phoenix mutation state that is
+ // never
+ // committed or cleared between rows, so the replayed total is an artifact of that helper
+ // rather
// than the "2 + 5" rows requested (testProcessLogFileBatchSizeLogic pins an exact count on a
// 1:1 record model). We also do not assert the mutationsPerFile histogram here:
- // MutableHistogram.getMax() reads FastLongHistogram bins that the metrics system resets to 0 on
+ // MutableHistogram.getMax() reads FastLongHistogram bins that the metrics system resets to 0
+ // on
// its periodic snapshot(), so a positive max is not stable to read after a multi-second real
// replay -- only the counter (a MutableFastCounter, never reset by snapshot) is reliable.
assertTrue("A successful multi-mutation file must record positive replay throughput",
@@ -1010,7 +1013,8 @@ public class ReplicationLogProcessorTestIT extends ParallelStatsDisabledIT {
*/
@Test
public void testProcessLogFileMidReplayFailureDoesNotCountMutations() throws Exception {
- final Path filePath = new Path(testFolder.newFile("testProcessLogFileMidReplayFailure").toURI());
+ final Path filePath =
+ new Path(testFolder.newFile("testProcessLogFileMidReplayFailure").toURI());
final String tableNameString = "T_" + generateUniqueName();
LogFileWriter writer = initLogFileWriter(filePath);
// Six single-mutation records with batch size 3 -> two full flushes; the second one throws.
(phoenix) 01/02: PHOENIX-7874 Addendum Trim redundant per-batch metric work on the indexed write path
Posted by tk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
tkhurana pushed a commit to branch PHOENIX-7562-feature-new
in repository https://gitbox.apache.org/repos/asf/phoenix.git
commit 10d06ad2f605f2ab8348308a44ad346ec23d8d9a
Author: tkhurana <kh...@gmail.com>
AuthorDate: Tue Sep 15 11:40:58 2026 -0700
PHOENIX-7874 Addendum Trim redundant per-batch metric work on the indexed write path
Two allocation/CPU cleanups that fire on every indexed batch:
- MetricsIndexerSourceImpl: memoize the resolved per-(table, base-metric)
MetricHistogram/MutableFastCounter handles instead of rebuilding the
"base.table" name string and re-resolving the registry handle on every
update. The (base, table) set is small and bounded and the handles are
stable for the life of the source, so steady-state per-batch allocation at
getCounterName drops to zero. Metric names, values, and JMX output are
unchanged.
- IndexRegionObserver: doPre/doPost emitted updatePreIndexUpdateTime /
updatePostIndexUpdateTime unconditionally, even when doIndexWritesWithExceptions
short-circuited with no index updates to write -- recording a near-zero sample
every batch on tables with no pre/post index work. doIndexWritesWithExceptions
now returns whether it actually wrote, and the callers only record the timing
metric when it did. Failure metrics are unchanged.
---
.../index/metrics/MetricsIndexerSourceImpl.java | 21 +++++++++++++------
.../phoenix/hbase/index/IndexRegionObserver.java | 24 ++++++++++++++--------
2 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java b/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
index d6fa330310..e3aea4ae6a 100644
--- a/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
+++ b/phoenix-core-client/src/main/java/org/apache/phoenix/hbase/index/metrics/MetricsIndexerSourceImpl.java
@@ -17,6 +17,7 @@
*/
package org.apache.phoenix.hbase.index.metrics;
+import java.util.concurrent.ConcurrentHashMap;
import org.apache.hadoop.hbase.metrics.BaseSourceImpl;
import org.apache.hadoop.metrics2.MetricHistogram;
import org.apache.hadoop.metrics2.lib.MutableFastCounter;
@@ -49,6 +50,14 @@ public class MetricsIndexerSourceImpl extends BaseSourceImpl implements MetricsI
private final MutableFastCounter preIndexUpdateFailures;
private final MutableFastCounter postIndexUpdateFailures;
+ // Resolved per-(table, base-metric) handles, memoized so the name concat and registry lookup
+ // run once per pair instead of on every per-batch metric update. Keyed tableName -> baseName ->
+ // handle; the (base, table) set is small and bounded, matching the registry's own retention.
+ private final ConcurrentHashMap<String,
+ ConcurrentHashMap<String, MetricHistogram>> tableHistograms = new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String,
+ ConcurrentHashMap<String, MutableFastCounter>> tableCounters = new ConcurrentHashMap<>();
+
public MetricsIndexerSourceImpl() {
this(METRICS_NAME, METRICS_DESCRIPTION, METRICS_CONTEXT, METRICS_JMX_CONTEXT);
}
@@ -225,15 +234,15 @@ public class MetricsIndexerSourceImpl extends BaseSourceImpl implements MetricsI
}
private void incrementTableSpecificCounter(String baseCounterName, String tableName) {
- MutableFastCounter indexSpecificCounter =
- getMetricsRegistry().getCounter(getCounterName(baseCounterName, tableName), 0);
- indexSpecificCounter.incr();
+ tableCounters.computeIfAbsent(tableName, k -> new ConcurrentHashMap<>())
+ .computeIfAbsent(baseCounterName,
+ b -> getMetricsRegistry().getCounter(getCounterName(b, tableName), 0))
+ .incr();
}
private void incrementTableSpecificHistogram(String baseCounterName, String tableName, long t) {
- MetricHistogram tableSpecificHistogram =
- getMetricsRegistry().getHistogram(getCounterName(baseCounterName, tableName));
- tableSpecificHistogram.add(t);
+ tableHistograms.computeIfAbsent(tableName, k -> new ConcurrentHashMap<>()).computeIfAbsent(
+ baseCounterName, b -> getMetricsRegistry().getHistogram(getCounterName(b, tableName))).add(t);
}
private String getCounterName(String baseCounterName, String tableName) {
diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
index a762a77467..dd887cc73f 100644
--- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
+++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/IndexRegionObserver.java
@@ -3052,9 +3052,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
throw new DoNotRetryIOException(
"Simulating the last (i.e., post) index table write failure");
}
- doIndexWritesWithExceptions(context, true);
- metricSource.updatePostIndexUpdateTime(dataTableName,
- EnvironmentEdgeManager.currentTimeMillis() - start);
+ // Only record the post-index-update time when there were actually index updates to write;
+ // otherwise every batch with no post index updates emits a near-zero sample.
+ if (doIndexWritesWithExceptions(context, true)) {
+ metricSource.updatePostIndexUpdateTime(dataTableName,
+ EnvironmentEdgeManager.currentTimeMillis() - start);
+ }
} catch (Throwable e) {
metricSource.updatePostIndexUpdateFailureTime(dataTableName,
EnvironmentEdgeManager.currentTimeMillis() - start);
@@ -3063,14 +3066,15 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
}
}
- private void doIndexWritesWithExceptions(BatchMutateContext context, boolean post)
+ /** Returns true if index updates were actually written, false if there was no work to do. */
+ private boolean doIndexWritesWithExceptions(BatchMutateContext context, boolean post)
throws IOException {
ListMultimap<HTableInterfaceReference, Mutation> indexUpdates =
post ? context.postIndexUpdates : context.preIndexUpdates;
// short circuit, if we don't need to do any work
if (context == null || indexUpdates == null || indexUpdates.isEmpty()) {
- return;
+ return false;
}
// get the current span, or just use a null-span to avoid a bunch of if statements
@@ -3088,6 +3092,7 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
preWriter.write(indexUpdates, false, context.clientVersion);
}
}
+ return true;
}
private void removePendingRows(BatchMutateContext context) {
@@ -3107,9 +3112,12 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver {
throw new DoNotRetryIOException(
"Simulating the first (i.e., pre) index table write failure");
}
- doIndexWritesWithExceptions(context, false);
- metricSource.updatePreIndexUpdateTime(dataTableName,
- EnvironmentEdgeManager.currentTimeMillis() - start);
+ // Only record the pre-index-update time when there were actually index updates to write;
+ // otherwise every batch with no pre index updates emits a near-zero sample.
+ if (doIndexWritesWithExceptions(context, false)) {
+ metricSource.updatePreIndexUpdateTime(dataTableName,
+ EnvironmentEdgeManager.currentTimeMillis() - start);
+ }
} catch (Throwable e) {
metricSource.updatePreIndexUpdateFailureTime(dataTableName,
EnvironmentEdgeManager.currentTimeMillis() - start);