From 76867899e7836515dc0430adc8061d51adef9396 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Mon, 6 Jul 2026 10:29:57 -0700 Subject: [PATCH 1/6] PHOENIX-7874 Eliminate replicating index mutations; regenerate on standby Index tables no longer generate their own replication-log records. Instead the active ships only the data-table record plus a per-(row, ts) PRE_IMAGE (the prior row state it observed at lock time), and the standby regenerates every index entry from that record on the replay path. This removes the out-of-order replay hazard: because each replicated batch carries the exact prior-row snapshot it was built against, the standby derives each (row, ts) group's next state from its own shipped pre-image instead of a region scan, so shard files can be replayed in any order (or in parallel) and each group is an independent reproduction of the active preBatchMutate. It also fixes CDC/global/local index rowkeys that embed the encoded data-table region name (PARTITION_ID), which differs between clusters -- the standby rebuilds the rowkey with its own region name. Main changes: - Cell-oriented replication log format (codec + record + MutationCellGrouper) carrying data cells plus a METAFAMILY pre-image cell per row. - IndexRegionObserver: active-side pre-image capture; a forked standby replay path (prepareReplicatedIndexMutations) that groups by (row, ts) and builds index updates from the shipped pre-image. - PreImageLocalTable: a LocalHBaseState that serves the local-index builder's prior-row-state from the shipped pre-image instead of a region scan; new IndexBuildManager.getIndexUpdates overload accepts it. - ReplicationLogProcessor: replay a record as one indivisible batch; do not retry DoNotRetryIOException. Coverage: global, local, uncovered, atomic, conditional-TTL, CDC (plain and behind an EVENTUAL secondary index), concurrent/out-of-order replay, and unit tests for (row, ts) grouping isolation and Put/Delete decomposition. --- .../hbase/index/IndexRegionObserver.java | 920 +++++++++++---- .../index/builder/IndexBuildManager.java | 21 +- .../covered/data/PreImageLocalTable.java | 91 ++ .../replication/MutationCellGrouper.java | 122 ++ .../phoenix/replication/ReplicationLog.java | 4 +- .../replication/ReplicationLogGroup.java | 72 +- .../phoenix/replication/log/LogFile.java | 14 + .../replication/log/LogFileRecord.java | 19 +- .../replication/log/LogFileWriter.java | 12 +- .../reader/ReplicationLogProcessor.java | 53 +- ...tTrackingParallelWriterIndexCommitter.java | 25 +- .../ReplicationLogGroupBaseIT.java | 527 +++++++++ .../ReplicationLogGroupEventualIndexIT.java | 144 +++ ...gGroupEventualIndexWithSerializeCDCIT.java | 46 + .../replication/ReplicationLogGroupIT.java | 1010 ++++++++++++----- .../reader/ReplicationLogProcessorTestIT.java | 313 ++++- .../index/IndexRegionObserverReplayTest.java | 301 +++++ .../replication/MutationCellGrouperTest.java | 511 ++++++--- .../replication/ReplicationLogGroupTest.java | 116 +- .../replication/log/LogFileCodecTest.java | 99 ++ .../replication/log/LogFileTestUtil.java | 8 +- 21 files changed, 3665 insertions(+), 763 deletions(-) create mode 100644 phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupBaseIT.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexIT.java create mode 100644 phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexWithSerializeCDCIT.java create mode 100644 phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java 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 f5063fc7e1f..4b7bc8ef0f7 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 @@ -42,6 +42,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -56,6 +57,8 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellBuilderFactory; +import org.apache.hadoop.hbase.CellBuilderType; import org.apache.hadoop.hbase.CellComparator; import org.apache.hadoop.hbase.CellScanner; import org.apache.hadoop.hbase.CellUtil; @@ -114,6 +117,9 @@ import org.apache.phoenix.hbase.index.builder.IndexBuildManager; import org.apache.phoenix.hbase.index.builder.IndexBuilder; import org.apache.phoenix.hbase.index.covered.IndexMetaData; +import org.apache.phoenix.hbase.index.covered.data.CachedLocalTable; +import org.apache.phoenix.hbase.index.covered.data.LocalHBaseState; +import org.apache.phoenix.hbase.index.covered.data.PreImageLocalTable; import org.apache.phoenix.hbase.index.covered.update.ColumnReference; import org.apache.phoenix.hbase.index.metrics.MetricsHaBypassSourceFactory; import org.apache.phoenix.hbase.index.metrics.MetricsIndexerSource; @@ -121,7 +127,6 @@ import org.apache.phoenix.hbase.index.table.HTableInterfaceReference; import org.apache.phoenix.hbase.index.util.GenericKeyValueBuilder; import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; -import org.apache.phoenix.hbase.index.wal.IndexedKeyValue; import org.apache.phoenix.hbase.index.write.IndexWriter; import org.apache.phoenix.hbase.index.write.LazyParallelWriterIndexCommitter; import org.apache.phoenix.index.IndexMaintainer; @@ -163,6 +168,7 @@ import org.slf4j.LoggerFactory; import org.xerial.snappy.Snappy; +import org.apache.phoenix.thirdparty.com.google.common.annotations.VisibleForTesting; import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions; import org.apache.phoenix.thirdparty.com.google.common.collect.ArrayListMultimap; import org.apache.phoenix.thirdparty.com.google.common.collect.ListMultimap; @@ -250,6 +256,26 @@ public class IndexRegionObserver implements RegionCoprocessor, RegionObserver { public static final String PHOENIX_INDEX_CDC_MUTATION_SERIALIZE = "phoenix.index.cdc.mutation.serialize"; public static final boolean DEFAULT_PHOENIX_INDEX_CDC_MUTATION_SERIALIZE = false; + // Generic marker attribute set on every mutation produced by the standby reader from a + // replication-log record. Value is opaque (presence is the signal). Detected in + // preBatchMutateWithExceptions to set context.isReplication, which gates the primary-side clock + // work (getBatchTimestamp/setTimestamps) off so the cell timestamps shipped from the active + // cluster are preserved. Never set on primary-side mutations. + public static final String REPLICATED_MUTATION = "_ReplicatedMutation"; + // Per-row mutation attribute that the standby reader synthesizes from the pre-image cell and + // attaches to each reconstructed mutation when its row had a pre-image entry. Value is the + // PB-encoded primary-side Put (or empty bytes when the primary observed an empty row at lock + // time). IRO on the standby consumes this to derive each (row, ts) group's data-row state and + // write index updates directly, instead of scanning the data table — that scan is unsafe under + // out-of-order replay. Absent when the row had no pre-image (e.g. local-index-only or pure-data + // tables on the active). + public static final String PRE_IMAGE = "_PhoenixPreImage"; + // Qualifier for the per-row pre-image cell injected into the replication cell stream and the + // WAL edit at PRE phase. Cells with this (METAFAMILY, qualifier) pair carry a serialized PB Put + // representing the row's state on the primary before the current batch was applied. The standby + // reader peels these cells off and attaches the bytes as {@link #PRE_IMAGE} on the reconstructed + // mutation. + public static final byte[] PRE_IMAGE_WAL_QUALIFIER = Bytes.toBytes("_PhoenixPreImage"); /** * Class to represent pending data table rows @@ -333,6 +359,51 @@ public enum BatchMutatePhase { FAILED } + /** + * Composite key for {@link BatchMutateContext#cdcPreMutationsBytes} and + * {@link BatchMutateContext#cdcPostMutationsBytes} and for the standby's per-(row, ts) grouping. + * The active path always has ts == batchTimestamp, so the {@code (row, ts)} key behaves like + * {@code (row)} on the active. The standby can have multiple entries per row when records from + * two active-side batches for the same row coalesce in one mini-batch. + */ + public static final class RowTsKey { + private final ImmutableBytesPtr row; + private final long ts; + + public RowTsKey(ImmutableBytesPtr row, long ts) { + this.row = row; + this.ts = ts; + } + + public ImmutableBytesPtr getRow() { + return row; + } + + public long getTs() { + return ts; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof RowTsKey)) { + return false; + } + RowTsKey other = (RowTsKey) o; + return ts == other.ts && row.equals(other.row); + } + + @Override + public int hashCode() { + return 31 * row.hashCode() + Long.hashCode(ts); + } + + @Override + public String toString() { + return "RowTsKey [row=" + Bytes.toStringBinary(row.copyBytesIfNecessary()) + ", ts=" + ts + + "]"; + } + } + // Hack to get around not being able to save any state between // coprocessor calls. TODO: remove after HBASE-18127 when available @@ -361,15 +432,16 @@ public static class BatchMutateContext { // The collection of candidate index mutations that will be applied after the data table // mutations. private ListMultimap> indexUpdates; - // Map of data table row key to IndexMutations bytes - // containing pre-index mutations for eventually consistent indexes - // (index mutations with UNVERIFIED Puts only, no Deletes) - private Map cdcPreMutationsBytes; - // Map of data table row key to IndexMutations bytes - // containing post-index mutations for eventually consistent indexes - // (index mutations with VERIFIED Puts for covered, - // no Put mutations for uncovered, and Deletes if needed) - private Map cdcPostMutationsBytes; + // 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 + // active path each row produces exactly one entry (ts == batchTimestamp), so lookup is + // unchanged. + private Map cdcPreMutationsBytes; + // Map of (data table row key, group ts) to IndexMutations bytes containing post-index + // mutations for eventually consistent indexes (VERIFIED Puts for covered, no Put mutations + // for uncovered, and Deletes if needed). + private Map cdcPostMutationsBytes; private List rowLocks = Lists.newArrayListWithExpectedSize(QueryServicesOptions.DEFAULT_MUTATE_BATCH_SIZE); // TreeSet to improve locking efficiency and avoid deadlock (PHOENIX-6871 and HBASE-17924) @@ -405,8 +477,28 @@ public static class BatchMutateContext { private boolean returnOldRow; private boolean hasConditionalTTL; // table has Conditional TTL private boolean immutableRows; + // True when this batch was produced by the standby reader from a replication-log record (i.e. + // every mutation carries the {@link IndexRegionObserver#REPLICATED_MUTATION} marker). Batch- + // uniform by construction: the standby reader stamps every reconstructed mutation, so checking + // the first one is sufficient. The standby uses this to skip the data-table scan in the PRE + // phase (pre-image cells are carried as the per-row {@link IndexRegionObserver#PRE_IMAGE} + // attribute when the table has a global/uncovered/transform index — same schema on both + // clusters, so when we're inside that branch on the standby we always have pre-images). + private boolean isReplication; // HAGroup associated with the batch private Optional logGroup = Optional.empty(); + // Per-row cell buckets captured in PRE phase that will be appended to the replication log in + // postBatchMutateIndispensably's replicateMutations. Captured unconditionally for every + // non-ignored mutation right after setTimestamps; later, when prepareDataRowStates runs (i.e. + // when the table has a global, uncovered, or transform index), the per-row pre-image cell is + // appended to the same row's bucket. Insertion order = batch order so cell ordering across + // rows is preserved on flatten. Null until captured. Replication attributes are NOT stored + // here; they are recomputed from originalMutations in replicateMutations. + private LinkedHashMap> replicationCellsByRow; + // Per-(row, ts) groups folded from this replicated batch, computed once and shared by the + // global-index path (prepareReplicatedIndexMutations) and the local-index path. Null until + // first built; only populated on the standby replay path (isReplication). + private List replicatedRowGroups; public BatchMutateContext() { this.clientVersion = 0; @@ -824,9 +916,8 @@ private Optional getHAGroupFromBatch(RegionCoprocessorEnvir * @return HA group if present or empty if missing */ private Optional getHAGroupFromWALKey(RegionCoprocessorEnvironment env, - org.apache.hadoop.hbase.wal.WALKey logKey) throws IOException { - byte[] haGroupName = - logKey.getExtendedAttribute(BaseScannerRegionObserverConstants.HA_GROUP_NAME_ATTRIB); + Map walKeyAttrs) throws IOException { + byte[] haGroupName = walKeyAttrs.get(BaseScannerRegionObserverConstants.HA_GROUP_NAME_ATTRIB); if (haGroupName != null) { ReplicationLogGroup logGroup = ReplicationLogGroup.get(env.getConfiguration(), env.getServerName(), Bytes.toString(haGroupName), abortable); @@ -846,13 +937,15 @@ public void preWALRestore( if (!shouldReplicate) { return; } - Optional logGroup = getHAGroupFromWALKey(ctx.getEnvironment(), logKey); + Map walKeyAttrs = getAttributeValuesFromWALKey(logKey); + Optional logGroup = + getHAGroupFromWALKey(ctx.getEnvironment(), walKeyAttrs); if (!logGroup.isPresent()) { return; } long start = EnvironmentEdgeManager.currentTimeMillis(); try { - replicateEditOnWALRestore(logGroup.get(), logKey, logEdit); + replicateEditOnWALRestore(logGroup.get(), logKey, walKeyAttrs, logEdit); } finally { long duration = EnvironmentEdgeManager.currentTimeMillis() - start; metricSource.updatePreWALRestoreTime(dataTableName, duration); @@ -860,32 +953,30 @@ public void preWALRestore( } /** - * A batch of mutations is recorded in a single WAL edit so a WAL edit can have cells belonging to - * multiple rows. Further, for one mutation the WAL edit contains the individual cells that are - * part of the mutation. + * Forward the WAL edit's cell stream — including any METAFAMILY pre-image cells that were + * injected at PRE phase — to the replication log as a single batch. The standby reader peels the + * pre-image cells and reconstructs Put/Delete mutations on the way out. Ignored mutations were + * filtered out of {@code replicationCellsByRow} during {@code captureReplicationCells}, so the + * WAL edit already excludes them; no per-mutation filtering is required here. * @param logGroup HA Group * @param logKey WAL log key * @param logEdit WAL edit record */ private void replicateEditOnWALRestore(ReplicationLogGroup logGroup, WALKey logKey, - WALEdit logEdit) throws IOException { - List regularCells = new ArrayList<>(); - for (Cell kv : logEdit.getCells()) { - if (kv instanceof IndexedKeyValue) { - IndexedKeyValue ikv = (IndexedKeyValue) kv; - logGroup.append(Bytes.toString(ikv.getIndexTable()), -1, ikv.getMutation()); - } else { - regularCells.add(kv); - } + Map walKeyAttrs, WALEdit logEdit) throws IOException { + String tableName = logKey.getTableName().getNameAsString(); + List cells = logEdit.getCells(); + if (cells == null || cells.isEmpty()) { + return; } - if (!regularCells.isEmpty()) { - String tableName = logKey.getTableName().getNameAsString(); - for (Mutation split : MutationCellGrouper.splitCellsIntoMutations(regularCells)) { - if (!this.ignoreReplicationFilter.test(split)) { - logGroup.append(tableName, -1, split); - } + Map replicationAttrs = new HashMap<>(); + for (String attrKey : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { + byte[] val = walKeyAttrs.get(attrKey); + if (val != null) { + replicationAttrs.put(attrKey, val); } } + logGroup.append(tableName, -1, cells, replicationAttrs); logGroup.sync(); } @@ -1234,19 +1325,7 @@ private boolean applyOnePendingDeleteMutation(BatchMutateContext context, Delete } } - for (List cells : delete.getFamilyCellMap().values()) { - for (Cell cell : cells) { - switch (cell.getType()) { - case DeleteFamily: - case DeleteFamilyVersion: - nextDataRowState.getFamilyCellMap().remove(CellUtil.cloneFamily(cell)); - break; - case DeleteColumn: - case Delete: - removeColumn(nextDataRowState, cell); - } - } - } + applyDeleteCells(delete, nextDataRowState); if (nextDataRowState != null && nextDataRowState.getFamilyCellMap().size() == 0) { dataRowState.setSecond(null); } @@ -1306,17 +1385,402 @@ private void prepareDataRowStates(ObserverContext applyPendingDeleteMutations(miniBatchOp, context); } + /** + * One {@code (row, ts)} group of a replicated mini-batch on the standby: the group's mutations, + * the per-row pre-image the active shipped for that batch, and the data-row state derived by + * folding the group's cells onto that pre-image. Built once by {@link #buildReplicatedRowGroups} + * and consumed by both the global-index path ({@link #prepareReplicatedIndexMutations}, which + * uses {@code preImage} + {@code nextState}) and the local-index path (which uses + * {@code preImage} as the builder's prior row state). Different {@code (row, ts)} groups for the + * same row are kept separate — that's how the standby recovers the active-batch boundary the + * reader's coalescing can erase. + */ + static final class ReplicatedRowGroup { + final ImmutableBytesPtr row; + final long ts; + final List mutations; + final Put preImage; + final Put nextState; + + ReplicatedRowGroup(ImmutableBytesPtr row, long ts, List mutations, Put preImage, + Put nextState) { + this.row = row; + this.ts = ts; + this.mutations = mutations; + this.preImage = preImage; + this.nextState = nextState; + } + } + + /** + * Group already-enabled replicated mutations by {@code (row, ts)} and, for each group, decode the + * shipped {@link #PRE_IMAGE} (from the group's first mutation — the active wrote one pre-image + * cell per row per batch, so all mutations in a group share it) and fold the group's cells onto + * it to derive the next data-row state. Groups are returned in first-seen order. The caller is + * responsible for filtering out non-indexed mutations (e.g. via {@code builder.isEnabled}) before + * calling this. + */ + @VisibleForTesting + static List buildReplicatedRowGroups(List enabledMutations) + throws IOException { + LinkedHashMap> groups = new LinkedHashMap<>(); + for (Mutation m : enabledMutations) { + RowTsKey key = new RowTsKey(new ImmutableBytesPtr(m.getRow()), IndexUtil.getMaxTimestamp(m)); + groups.computeIfAbsent(key, k -> new ArrayList<>()).add(m); + } + List result = new ArrayList<>(groups.size()); + for (Map.Entry> entry : groups.entrySet()) { + List groupMutations = entry.getValue(); + Put preImage = decodePreImage(groupMutations.get(0)); + Put nextState = deriveNextState(preImage, groupMutations); + result.add(new ReplicatedRowGroup(entry.getKey().getRow(), entry.getKey().getTs(), + groupMutations, preImage, nextState)); + } + return result; + } + + /** + * Collect the mini-batch's index-enabled mutations into a list, preserving order. + */ + private List enabledMutations(MiniBatchOperationInProgress miniBatchOp) { + List enabled = new ArrayList<>(miniBatchOp.size()); + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (this.builder.isEnabled(m)) { + enabled.add(m); + } + } + return enabled; + } + + /** + * Fold this replicated batch into {@code (row, ts)} groups, once per batch. A table with both a + * global and a local index reaches this from both index paths; caching on the context avoids + * re-walking the mini-batch and re-parsing every group's pre-image protobuf a second time. + */ + private List getReplicatedRowGroups( + MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) + throws IOException { + if (context.replicatedRowGroups == null) { + context.replicatedRowGroups = buildReplicatedRowGroups(enabledMutations(miniBatchOp)); + } + return context.replicatedRowGroups; + } + + /** + * Build the local-index replay inputs from already-grouped replicated mutations: one uniform-ts + * {@link MultiMutation} per {@code (row, ts)} group (the pending mutations the local builder + * processes) and, as a side effect, populate {@code preImageCellsByRowTs} mapping each group's + * {@code (row, ts)} to the pre-image cells the active shipped (the builder's prior row state). + * The map is keyed by {@code (row, ts)} so a row recurring across concatenated active batches + * gets each batch's own pre-image rather than collapsing to the earliest. A {@code null} + * pre-image (active saw an empty row) maps to a {@code null} cell list — the documented "no prior + * row" sentinel. The returned {@link MultiMutation}s mirror what {@code groupMutations} produces + * on the active, but grouped by {@code (row, ts)} so each is uniform-ts as + * {@code NonTxIndexBuilder} requires. + */ + private Collection buildReplayLocalIndexInputs( + List groups, Map> preImageCellsByRowTs) { + List pending = new ArrayList<>(groups.size()); + for (ReplicatedRowGroup group : groups) { + MultiMutation mm = new MultiMutation(group.row); + for (Mutation m : group.mutations) { + mm.addAll(m); + } + pending.add(mm); + List priorCells = + group.preImage == null ? null : MutationCellGrouper.flattenCells(group.preImage); + preImageCellsByRowTs.put(new RowTsKey(group.row, group.ts), priorCells); + } + return pending; + } + + /** + * Standby-side counterpart to {@link #prepareIndexMutations}. Groups the mini-batch's mutations + * by {@code (row, ts)} so all mutations from the same active-side batch on the same row are + * processed together against one shared pre-image. Different {@code (row, ts)} groups for the + * same row produce separate index updates — that's how the standby recovers the active-batch + * boundary that the reader's coalescing can erase. + *

+ * For each group: decode {@link #PRE_IMAGE} from the first mutation (all mutations in a group + * share one pre-image because the active wrote one pre-image cell per row per batch), apply the + * group's cells on top to derive {@code nextDataRowState}, then call + * {@link #generateIndexMutationsForRow} with the group's ts so the resulting index Mutation cells + * carry the correct timestamp. + *

+ * Skips {@code getCurrentRowStates} (unsafe under out-of-order replay) and writes directly to + * {@code context.indexUpdates}. + */ + private void prepareReplicatedIndexMutations(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context, List maintainers) throws IOException { + List> indexTables = + buildIndexTablesList(maintainers); + for (ReplicatedRowGroup group : getReplicatedRowGroups(miniBatchOp, context)) { + if (group.preImage == null && group.nextState == null) { + continue; + } + ListMultimap idxUpdates = ArrayListMultimap.create(); + generateIndexMutationsForRow(group.row, group.preImage, group.nextState, group.ts, + encodedRegionName, QueryConstants.UNVERIFIED_BYTES, indexTables, idxUpdates); + for (Map.Entry idxUpdate : idxUpdates.entries()) { + context.indexUpdates.put(idxUpdate.getKey(), + new Pair<>(idxUpdate.getValue(), group.row.get())); + } + } + } + + /** + * Decodes the {@link #PRE_IMAGE} attribute on a standby-side replicated mutation. Returns + * {@code null} when the active observed an empty row at lock time (sentinel: zero-length value). + * Throws when the attribute is absent — that signals a contract violation: an indexed-table + * mutation arrived on the standby with no pre-image, which should never happen because the active + * runs {@code prepareDataRowStates} (and writes a pre-image cell) for every table with a + * global/uncovered/transform/CDC index. + */ + @VisibleForTesting + static Put decodePreImage(Mutation m) throws IOException { + byte[] preImageBytes = m.getAttribute(PRE_IMAGE); + if (preImageBytes == null) { + throw new DoNotRetryIOException("Replicated mutation on indexed table is missing the " + + PRE_IMAGE + " attribute: row=" + Bytes.toStringBinary(m.getRow())); + } + if (preImageBytes.length == 0) { + return null; + } + return ProtobufUtil.toPut(ClientProtos.MutationProto.parseFrom(preImageBytes)); + } + + /** + * Encode a pre-image Put into the bytes carried by the {@link #PRE_IMAGE_WAL_QUALIFIER} cell + * (and, after the reader peels it off, the {@link #PRE_IMAGE} attribute). A {@code null} + * pre-image means the active side saw an empty row; it encodes to a zero-length array, the + * sentinel {@link #decodePreImage} maps back to {@code null}. + */ + @VisibleForTesting + static byte[] encodePreImage(Put preImage) throws IOException { + return preImage != null + ? ProtobufUtil.toMutation(ClientProtos.MutationProto.MutationType.PUT, preImage).toByteArray() + : HConstants.EMPTY_BYTE_ARRAY; + } + + /** + * Build the METAFAMILY pre-image cell that the active appends to the WAL edit (and that the + * reader peels off into the {@link #PRE_IMAGE} attribute). {@code priorState} is the row state + * the active observed at lock time; {@code null} encodes to the empty-row sentinel. + */ + @VisibleForTesting + public static Cell buildPreImageCell(byte[] row, Put priorState) throws IOException { + return CellBuilderFactory.create(CellBuilderType.SHALLOW_COPY).setRow(row) + .setFamily(WALEdit.METAFAMILY).setQualifier(PRE_IMAGE_WAL_QUALIFIER) + .setTimestamp(HConstants.LATEST_TIMESTAMP).setType(Cell.Type.Put) + .setValue(encodePreImage(priorState)).build(); + } + + /** + * Applies a Delete's cells to a Put, returning the resulting Put or {@code null} if the row goes + * empty. Used by {@link #prepareReplicatedIndexMutations} to derive {@code nextDataRowState} from + * a (preImage + cells) sequence within one (row, ts) group. + */ + @VisibleForTesting + static Put applyDeleteToPut(Delete delete, Put put) { + if (put == null) { + return null; + } + applyDeleteCells(delete, put); + return put.getFamilyCellMap().isEmpty() ? null : put; + } + + /** + * Applies a Delete's tombstone cells to {@code put} in place: DeleteFamily/DeleteFamilyVersion + * drop the whole family, DeleteColumn/Delete drop the column. Shared by {@link #applyDeleteToPut} + * (replay next-state derivation) and {@link #applyOnePendingDeleteMutation} (active next-state). + */ + private static void applyDeleteCells(Delete delete, Put put) { + for (List cells : delete.getFamilyCellMap().values()) { + for (Cell cell : cells) { + switch (cell.getType()) { + case DeleteFamily: + case DeleteFamilyVersion: + put.getFamilyCellMap().remove(CellUtil.cloneFamily(cell)); + break; + case DeleteColumn: + case Delete: + removeColumn(put, cell); + break; + default: + break; + } + } + } + } + + /** + * Fold a (row, ts) group's mutations onto its pre-image to derive the next data-row state. Puts + * are merged on top of the running state ({@code applyNew}); Deletes peel cells back out + * ({@link #applyDeleteToPut}). Returns {@code null} if there is no pre-image and the group never + * produces a Put, or if a Delete empties the row. Mirrors the single-row fold Phoenix performs on + * the active side when building {@code nextDataRowState}. + */ + @VisibleForTesting + static Put deriveNextState(Put preImage, List groupMutations) throws IOException { + Put nextState = preImage != null ? new Put(preImage) : null; + for (Mutation m : groupMutations) { + if (m instanceof Put) { + nextState = nextState != null ? applyNew((Put) m, nextState) : new Put((Put) m); + } else if (m instanceof Delete) { + nextState = applyDeleteToPut((Delete) m, nextState); + } + } + return nextState; + } + + /** + * Capture the cell stream that will be appended to the replication log. + *

+ * Called unconditionally from {@link #preBatchMutateWithExceptions} after {@code setTimestamps} + * so cells carry their final timestamps and any post-on-dup / post-conditional-TTL cells from + * earlier phases are reflected. Every Phoenix-write batch ships through Phoenix replication and + * lands in the standby's {@code preBatchMutate}, regardless of which indexes the active table + * has. Tables with global/uncovered/transform indexes get an additional pre-image cell appended + * later by {@link #capturePreImageCells} (called from {@link #prepareDataRowStates}); other + * tables ship data cells only and the standby IRO falls through to its primary code path. + *

+ * Each non-ignored mutation contributes its family cells to its row's bucket in + * {@code context.replicationCellsByRow}. CP-injected mutations visible at this call site (on-dup + * Deletes from {@link #addOnDupMutationsToBatch}, conditional-TTL masking Deletes from + * {@link #updateMutationsForConditionalTTL}) are appended to the same row's bucket. Local-index + * updates are NOT visible here: {@link #handleLocalIndexUpdates} runs later via + * {@code addOperationsFromCP(0, …)} so they correctly stay out of the replication log. + *

+ * The map's key set is the set of replicated rows (no separate set field needed); a row whose + * only mutation was filtered by {@code ignoreReplicationFilter} does not appear in the map. + */ + private void captureReplicationCells(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context) { + if (!shouldReplicate || ignoreSyncReplicationForTesting) { + return; + } + if (!context.logGroup.isPresent()) { + return; + } + LinkedHashMap> cellsByRow = new LinkedHashMap<>(); + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (ignoreReplicationFilter.test(m)) { + continue; + } + ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(m.getRow()); + List bucket = cellsByRow.computeIfAbsent(rowKeyPtr, k -> new ArrayList<>()); + for (List cells : m.getFamilyCellMap().values()) { + bucket.addAll(cells); + } + Mutation[] cpMutations = miniBatchOp.getOperationsFromCoprocessors(i); + if (cpMutations != null) { + for (Mutation cp : cpMutations) { + for (List cells : cp.getFamilyCellMap().values()) { + bucket.addAll(cells); + } + } + } + } + context.replicationCellsByRow = cellsByRow; + } + + /** + * Emit one pre-image cell per replicated row that has a {@code dataRowStates} entry. Appends the + * cell to the row's bucket in {@code context.replicationCellsByRow} and to + * {@code miniBatchOp.getWalEdit(0)} so the WAL-restore path ships the same payload as the + * synchronous {@link #replicateMutations} call. + *

+ * Pre-image bytes are PB-encoded {@link Put}; an empty value is the sentinel for "primary + * observed an empty row at lock time" so the standby can distinguish that from "no primary + * information shipped". Rows not in {@code dataRowStates} (e.g. row not visited) contribute no + * pre-image cell — the standby falls back to its no-pre-image code path for those rows. + *

+ * Called from {@link #preBatchMutateWithExceptions} immediately after + * {@link #prepareDataRowStates} returns, on the global/uncovered/transform-index branch only. For + * tables without such indexes, {@code captureReplicationCells} alone produces the cell stream (no + * pre-image cells). + */ + private void capturePreImageCells(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context) throws IOException { + if (context.replicationCellsByRow == null || context.replicationCellsByRow.isEmpty()) { + return; + } + WALEdit walEdit = miniBatchOp.getWalEdit(0); + if (walEdit == null) { + walEdit = new WALEdit(); + } + for (Map.Entry> entry : context.replicationCellsByRow + .entrySet()) { + ImmutableBytesPtr rowKeyPtr = entry.getKey(); + Pair rowState = context.dataRowStates.get(rowKeyPtr); + if (rowState == null) { + continue; + } + Cell preImageCell = buildPreImageCell(rowKeyPtr.copyBytes(), rowState.getFirst()); + entry.getValue().add(preImageCell); + walEdit.add(preImageCell); + } + if (!walEdit.isEmpty()) { + miniBatchOp.setWalEdit(0, walEdit); + } + } + + /** + * Active-side pre-image capture for a local-only replicated table. Such a table has no + * global/uncovered/transform index, so it never enters the branch that runs + * {@link #prepareDataRowStates} + {@link #capturePreImageCells}; without this it would ship no + * pre-image and the standby's {@link PreImageLocalTable} would have nothing to read. The local + * index build already reads the prior row state through {@code cachedLocalTable} (a region scan + * scoped to index-relevant columns), so we reuse that exact state as the pre-image — no extra + * scan, and the same column scope the standby's local builder consumes. {@code dataRowStates} is + * populated with the prior row as {@code first} (a {@code null} prior row encodes to the + * empty-row sentinel) so {@link #capturePreImageCells} can emit one pre-image cell per replicated + * row. + */ + private void captureLocalIndexPreImageCells(MiniBatchOperationInProgress miniBatchOp, + BatchMutateContext context, Collection groupedMutations, + CachedLocalTable cachedLocalTable) throws IOException { + if (context.replicationCellsByRow == null || context.replicationCellsByRow.isEmpty()) { + return; + } + context.dataRowStates = new HashMap<>(groupedMutations.size()); + for (Mutation m : groupedMutations) { + List priorCells = + cachedLocalTable.getCurrentRowState(m, Collections. emptyList(), false); + Put priorState = null; + if (priorCells != null && !priorCells.isEmpty()) { + priorState = new Put(m.getRow()); + for (Cell cell : priorCells) { + priorState.add(cell); + } + } + context.dataRowStates.put(new ImmutableBytesPtr(m.getRow()), new Pair<>(priorState, null)); + } + capturePreImageCells(miniBatchOp, context); + } + /** * The index update generation for local indexes uses the existing index update generation code * (i.e., the {@link IndexBuilder} implementation). */ private void handleLocalIndexUpdates(TableName table, MiniBatchOperationInProgress miniBatchOp, - Collection pendingMutations, PhoenixIndexMetaData indexMetaData) - throws Throwable { + Collection pendingMutations, PhoenixIndexMetaData indexMetaData, + LocalHBaseState localHBaseState) throws Throwable { ListMultimap> indexUpdates = ArrayListMultimap.> create(); - this.builder.getIndexUpdates(indexUpdates, miniBatchOp, pendingMutations, indexMetaData); + if (localHBaseState != null) { + // Caller supplied the prior-row-state source: the standby replay passes a PreImageLocalTable + // (prior state from the shipped PRE_IMAGE, not a region scan), and the active local-only + // replicated path passes the CachedLocalTable it already built (so the pre-image capture and + // the index build share one scan). + this.builder.getIndexUpdates(indexUpdates, miniBatchOp, pendingMutations, indexMetaData, + localHBaseState); + } else { + this.builder.getIndexUpdates(indexUpdates, miniBatchOp, pendingMutations, indexMetaData); + } byte[] tableName = table.getName(); HTableInterfaceReference hTableInterfaceReference = new HTableInterfaceReference(new ImmutableBytesPtr(tableName)); @@ -1558,23 +2022,9 @@ public static void generateIndexMutationsForRow(ImmutableBytesPtr rowKeyPtr, * previous data row state with the pending row mutation. */ private void prepareIndexMutations(BatchMutateContext context, List maintainers, - long ts) throws IOException { + long batchTimestamp) throws IOException { List> indexTables = - new ArrayList<>(maintainers.size()); - for (IndexMaintainer indexMaintainer : maintainers) { - if (indexMaintainer.isLocalIndex()) { - continue; - } - if ( - !serializeCDCMutations && indexMaintainer.getIndexConsistency() != null - && indexMaintainer.getIndexConsistency().isAsynchronous() - ) { - continue; - } - HTableInterfaceReference hTableInterfaceReference = - new HTableInterfaceReference(new ImmutableBytesPtr(indexMaintainer.getIndexTableName())); - indexTables.add(new Pair<>(indexMaintainer, hTableInterfaceReference)); - } + buildIndexTablesList(maintainers); for (Map.Entry> entry : context.dataRowStates.entrySet()) { ImmutableBytesPtr rowKeyPtr = entry.getKey(); Pair dataRowState = entry.getValue(); @@ -1584,7 +2034,7 @@ private void prepareIndexMutations(BatchMutateContext context, List idxUpdates = ArrayListMultimap.create(); - generateIndexMutationsForRow(rowKeyPtr, currentDataRowState, nextDataRowState, ts, + generateIndexMutationsForRow(rowKeyPtr, currentDataRowState, nextDataRowState, batchTimestamp, encodedRegionName, QueryConstants.UNVERIFIED_BYTES, indexTables, idxUpdates); for (Map.Entry idxUpdate : idxUpdates.entries()) { context.indexUpdates.put(idxUpdate.getKey(), @@ -1593,6 +2043,27 @@ private void prepareIndexMutations(BatchMutateContext context, List> + buildIndexTablesList(List maintainers) { + List> indexTables = + new ArrayList<>(maintainers.size()); + for (IndexMaintainer indexMaintainer : maintainers) { + if (indexMaintainer.isLocalIndex()) { + continue; + } + if ( + !serializeCDCMutations && indexMaintainer.getIndexConsistency() != null + && indexMaintainer.getIndexConsistency().isAsynchronous() + ) { + continue; + } + HTableInterfaceReference hTableInterfaceReference = + new HTableInterfaceReference(new ImmutableBytesPtr(indexMaintainer.getIndexTableName())); + indexTables.add(new Pair<>(indexMaintainer, hTableInterfaceReference)); + } + return indexTables; + } + /** * This method prepares unverified index mutations which are applied to index tables before the * data table is updated. In the three-phase update approach, in phase 1, the status of existing @@ -1601,8 +2072,9 @@ private void prepareIndexMutations(BatchMutateContext context, List miniBatchOp, + BatchMutateContext context, long batchTimestamp, PhoenixIndexMetaData indexMetaData) + throws Throwable { List 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")) { @@ -1614,11 +2086,17 @@ private void preparePreIndexMutations(BatchMutateContext context, long batchTime // The rest of this method is for handling global index updates context.indexUpdates = ArrayListMultimap.> create(); - prepareIndexMutations(context, maintainers, batchTimestamp); + if (context.isReplication) { + // Replicated batches carry per-row pre-images and per-cell timestamps from the active. + // Group by (row, ts) so each active-side batch's mutations are processed against their own + // pre-image — recovers the active-batch boundary the reader's coalescing can erase. + prepareReplicatedIndexMutations(miniBatchOp, context, maintainers); + } else { + prepareIndexMutations(context, maintainers, batchTimestamp); + } if (serializeCDCMutations) { - prepareEventuallyConsistentIndexMutations(context, batchTimestamp, maintainers, - compressCDCMutations); + prepareEventuallyConsistentIndexMutations(context, maintainers, compressCDCMutations); } context.preIndexUpdates = ArrayListMultimap. create(); @@ -1638,14 +2116,14 @@ private void preparePreIndexMutations(BatchMutateContext context, long batchTime List> updates = context.indexUpdates.get(hTableInterfaceReference); for (Pair update : updates) { Mutation m = update.getFirst(); + long ts = IndexUtil.getMaxTimestamp(m); + RowTsKey cdcKey = new RowTsKey(new ImmutableBytesPtr(update.getSecond()), ts); if (m instanceof Put) { if (indexMaintainer.isCDCIndex() && context.cdcPreMutationsBytes != null) { - ImmutableBytesPtr dataRowKeyPtr = new ImmutableBytesPtr(update.getSecond()); - byte[] cdcMutationsBytes = context.cdcPreMutationsBytes.get(dataRowKeyPtr); + byte[] cdcMutationsBytes = context.cdcPreMutationsBytes.get(cdcKey); if (cdcMutationsBytes != null) { ((Put) m).addColumn(QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES, - QueryConstants.CDC_INDEX_PRE_MUTATIONS_CQ_BYTES, batchTimestamp, - cdcMutationsBytes); + QueryConstants.CDC_INDEX_PRE_MUTATIONS_CQ_BYTES, ts, cdcMutationsBytes); } } // This will be done before the data table row is updated (i.e., in the first write @@ -1656,15 +2134,12 @@ private void preparePreIndexMutations(BatchMutateContext context, long batchTime // row unverified again. Only do this for DeleteFamily // Set the status of the index row to "unverified" Put unverifiedPut = new Put(m.getRow()); - unverifiedPut.addColumn(emptyCF, emptyCQ, batchTimestamp, - QueryConstants.UNVERIFIED_BYTES); + unverifiedPut.addColumn(emptyCF, emptyCQ, ts, QueryConstants.UNVERIFIED_BYTES); if (indexMaintainer.isCDCIndex() && context.cdcPreMutationsBytes != null) { - ImmutableBytesPtr dataRowKeyPtr = new ImmutableBytesPtr(update.getSecond()); - byte[] cdcMutationsBytes = context.cdcPreMutationsBytes.get(dataRowKeyPtr); + byte[] cdcMutationsBytes = context.cdcPreMutationsBytes.get(cdcKey); if (cdcMutationsBytes != null) { unverifiedPut.addColumn(QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES, - QueryConstants.CDC_INDEX_PRE_MUTATIONS_CQ_BYTES, batchTimestamp, - cdcMutationsBytes); + QueryConstants.CDC_INDEX_PRE_MUTATIONS_CQ_BYTES, ts, cdcMutationsBytes); } } // This will be done before the data table row is updated (i.e., in the first write @@ -1678,21 +2153,21 @@ private void preparePreIndexMutations(BatchMutateContext context, long batchTime } /** - * Prepares pre-phase and post-phase cdc mutations for eventually consistent indexes. + * Prepares pre-phase and post-phase cdc mutations for eventually consistent indexes. Each + * resulting builder/bytes entry is keyed by {@code (dataRowKey, ts)} where ts is read from the + * index Mutation's own cells (already stamped by {@link #generateIndexMutationsForRow}). On the + * active path each row produces one entry (ts == batchTimestamp); on the standby's per-(row, ts) + * grouping, two batches for the same row produce two entries. * @param context batch mutate context. - * @param batchTimestamp the timestamp to use for mutations. * @param maintainers the list of index maintainers. * @param compressMutations whether to Snappy-compress the serialized proto bytes. * @throws IOException if there is an error. */ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext context, - long batchTimestamp, List maintainers, boolean compressMutations) - throws IOException { - // Store pre-index and post-index mutations for each data table rowkey - Map preBuilderMap = - new HashMap<>(); - Map postBuilderMap = - new HashMap<>(); + List maintainers, boolean compressMutations) throws IOException { + // Store pre-index and post-index mutations per (data row, ts) group. + Map preBuilderMap = new HashMap<>(); + Map postBuilderMap = new HashMap<>(); for (IndexMaintainer indexMaintainer : maintainers) { if ( @@ -1709,11 +2184,12 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext for (Pair update : updates) { Mutation m = update.getFirst(); byte[] dataRowKey = update.getSecond(); - ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(dataRowKey); - IndexMutationsProtos.IndexMutations.Builder preBuilder = preBuilderMap - .computeIfAbsent(rowKeyPtr, k -> IndexMutationsProtos.IndexMutations.newBuilder()); + long ts = IndexUtil.getMaxTimestamp(m); + RowTsKey key = new RowTsKey(new ImmutableBytesPtr(dataRowKey), ts); + IndexMutationsProtos.IndexMutations.Builder preBuilder = + preBuilderMap.computeIfAbsent(key, k -> IndexMutationsProtos.IndexMutations.newBuilder()); IndexMutationsProtos.IndexMutations.Builder postBuilder = postBuilderMap - .computeIfAbsent(rowKeyPtr, k -> IndexMutationsProtos.IndexMutations.newBuilder()); + .computeIfAbsent(key, k -> IndexMutationsProtos.IndexMutations.newBuilder()); if (m instanceof Put) { preBuilder.addTables(ByteString.copyFrom(indexMaintainer.getIndexTableName())); byte[] preMutation = @@ -1721,7 +2197,7 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext preBuilder.addMutations(ByteString.copyFrom(preMutation)); if (!indexMaintainer.isUncovered()) { Put verifiedPut = new Put(m.getRow()); - verifiedPut.addColumn(emptyCF, emptyCQ, batchTimestamp, QueryConstants.VERIFIED_BYTES); + verifiedPut.addColumn(emptyCF, emptyCQ, ts, QueryConstants.VERIFIED_BYTES); postBuilder.addTables(ByteString.copyFrom(indexMaintainer.getIndexTableName())); byte[] postMutation = ProtobufUtil .toMutation(ClientProtos.MutationProto.MutationType.PUT, verifiedPut).toByteArray(); @@ -1730,8 +2206,7 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext } else { if (IndexUtil.isDeleteFamily(m)) { Put unverifiedPut = new Put(m.getRow()); - unverifiedPut.addColumn(emptyCF, emptyCQ, batchTimestamp, - QueryConstants.UNVERIFIED_BYTES); + unverifiedPut.addColumn(emptyCF, emptyCQ, ts, QueryConstants.UNVERIFIED_BYTES); preBuilder.addTables(ByteString.copyFrom(indexMaintainer.getIndexTableName())); byte[] preMutation = ProtobufUtil .toMutation(ClientProtos.MutationProto.MutationType.PUT, unverifiedPut).toByteArray(); @@ -1747,9 +2222,9 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext if (!preBuilderMap.isEmpty()) { context.cdcPreMutationsBytes = new HashMap<>(); - for (Map.Entry entry : preBuilderMap.entrySet()) { - ImmutableBytesPtr rowKey = entry.getKey(); + for (Map.Entry entry : preBuilderMap + .entrySet()) { + RowTsKey key = entry.getKey(); IndexMutationsProtos.IndexMutations.Builder builder = entry.getValue(); if (builder.getTablesCount() != builder.getMutationsCount()) { throw new DoNotRetryIOException( @@ -1758,7 +2233,7 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext } if (builder.getTablesCount() > 0) { byte[] protoBytes = builder.build().toByteArray(); - context.cdcPreMutationsBytes.put(rowKey, + context.cdcPreMutationsBytes.put(key, compressMutations ? Snappy.compress(protoBytes) : protoBytes); } } @@ -1766,9 +2241,9 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext if (!postBuilderMap.isEmpty()) { context.cdcPostMutationsBytes = new HashMap<>(); - for (Map.Entry entry : postBuilderMap.entrySet()) { - ImmutableBytesPtr rowKey = entry.getKey(); + for (Map.Entry entry : postBuilderMap + .entrySet()) { + RowTsKey key = entry.getKey(); IndexMutationsProtos.IndexMutations.Builder builder = entry.getValue(); if (builder.getTablesCount() != builder.getMutationsCount()) { throw new DoNotRetryIOException( @@ -1777,7 +2252,7 @@ private static void prepareEventuallyConsistentIndexMutations(BatchMutateContext } if (builder.getTablesCount() > 0) { byte[] protoBytes = builder.build().toByteArray(); - context.cdcPostMutationsBytes.put(rowKey, + context.cdcPostMutationsBytes.put(key, compressMutations ? Snappy.compress(protoBytes) : protoBytes); } } @@ -1797,7 +2272,7 @@ protected PhoenixIndexMetaData getPhoenixIndexMetaData( return (PhoenixIndexMetaData) indexMetaData; } - private void preparePostIndexMutations(BatchMutateContext context, long batchTimestamp, + private void preparePostIndexMutations(BatchMutateContext context, PhoenixIndexMetaData indexMetaData) { context.postIndexUpdates = ArrayListMultimap. create(); List maintainers = indexMetaData.getIndexMaintainers(); @@ -1819,7 +2294,8 @@ private void preparePostIndexMutations(BatchMutateContext context, long batchTim if (!indexMaintainer.isUncovered()) { Put verifiedPut = new Put(m.getRow()); // Set the status of the index row to "verified" - verifiedPut.addColumn(emptyCF, emptyCQ, batchTimestamp, QueryConstants.VERIFIED_BYTES); + verifiedPut.addColumn(emptyCF, emptyCQ, IndexUtil.getMaxTimestamp(m), + QueryConstants.VERIFIED_BYTES); context.postIndexUpdates.put(hTableInterfaceReference, verifiedPut); } } else { @@ -1839,13 +2315,13 @@ private void preparePostIndexMutations(BatchMutateContext context, long batchTim for (Pair update : updates) { Mutation m = update.getFirst(); if (m instanceof Put) { - ImmutableBytesPtr dataRowKeyPtr = new ImmutableBytesPtr(update.getSecond()); - byte[] cdcMutationsBytes = context.cdcPostMutationsBytes.get(dataRowKeyPtr); + long ts = IndexUtil.getMaxTimestamp(m); + RowTsKey cdcKey = new RowTsKey(new ImmutableBytesPtr(update.getSecond()), ts); + byte[] cdcMutationsBytes = context.cdcPostMutationsBytes.get(cdcKey); if (cdcMutationsBytes != null) { Put postPut = new Put(m.getRow()); postPut.addColumn(QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES, - QueryConstants.CDC_INDEX_POST_MUTATIONS_CQ_BYTES, batchTimestamp, - cdcMutationsBytes); + QueryConstants.CDC_INDEX_POST_MUTATIONS_CQ_BYTES, ts, cdcMutationsBytes); context.postIndexUpdates.put(hTableInterfaceReference, postPut); } } @@ -2042,6 +2518,16 @@ public void preBatchMutateWithExceptions(ObserverContext mutations = groupMutations(miniBatchOp, context); - handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData); - } - if (failDataTableUpdatesForTesting) { - throw new DoNotRetryIOException("Simulating the data table write failure"); - } - } - - /** - * We need to add the index mutations to the data table's WAL to handle cases where the RS crashes - * before the postBatchMutateIndispensably hook is called where the mutations are synchronously - * replicated. This is needed because during WAL restore we don't have the IndexMaintainer object - * to generate the corresponding index mutations. - */ - private void addGlobalIndexMutationsToWAL(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context) { - if (!this.shouldReplicate) { - return; - } - - WALEdit edit = miniBatchOp.getWalEdit(0); - if (edit == null) { - edit = new WALEdit(); - miniBatchOp.setWalEdit(0, edit); - } - - if (context.preIndexUpdates != null) { - for (Map.Entry entry : context.preIndexUpdates - .entries()) { - if (this.ignoreReplicationFilter.test(entry.getValue())) { - continue; + // indexes). On the standby, group by (row, ts) so each replayed active-side batch's cells + // stay in their own uniform-ts mutation for NonTxIndexBuilder, and serve the builder's prior + // row state from each group's shipped PRE_IMAGE instead of a (not-yet-written) region scan. + if (context.isReplication) { + List groups = getReplicatedRowGroups(miniBatchOp, context); + Map> preImageCellsByRowTs = new HashMap<>(); + Collection mutations = + buildReplayLocalIndexInputs(groups, preImageCellsByRowTs); + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, + new PreImageLocalTable(preImageCellsByRowTs)); + } else { + Collection mutations = groupMutations(miniBatchOp, context); + // dataRowStates is populated only by the global/uncovered/transform/atomic/... branch, so a + // null map here means this table has only a local index and never ran the global-style + // pre-image capture. A non-null replicationCellsByRow means captureReplicationCells ran, + // i.e. this is a replicated active-side batch. Together they identify a replicated + // local-only table, which must ship a pre-image so the standby can regenerate its local + // index. Build the prior-state scan once and reuse it for both the pre-image capture and + // the index build. Mixed tables already captured a (superset) pre-image in the earlier + // branch; non-replicated local-only tables keep the unchanged path (no extra work). + if (context.dataRowStates == null && context.replicationCellsByRow != null) { + CachedLocalTable cachedLocalTable = + CachedLocalTable.build(mutations, indexMetaData, c.getEnvironment().getRegion()); + captureLocalIndexPreImageCells(miniBatchOp, context, mutations, cachedLocalTable); + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, cachedLocalTable); + } else { + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, null); } - // This creates cells of family type WALEdit.METAFAMILY which are not applied - // on restore - edit.add(IndexedKeyValue.newIndexedKeyValue(entry.getKey().get(), entry.getValue())); } } - - if (context.postIndexUpdates != null) { - for (Map.Entry entry : context.postIndexUpdates - .entries()) { - if (this.ignoreReplicationFilter.test(entry.getValue())) { - continue; - } - // This creates cells of family type WALEdit.METAFAMILY which are not applied - // on restore - edit.add(IndexedKeyValue.newIndexedKeyValue(entry.getKey().get(), entry.getValue())); - } + if (failDataTableUpdatesForTesting) { + throw new DoNotRetryIOException("Simulating the data table write failure"); } } @@ -2245,6 +2736,7 @@ public void preWALAppend(ObserverContext c, WALKey if (shouldReplicate) { BatchMutateContext context = getBatchMutateContext(c); appendHAGroupAttributeToWALKey(key, context); + appendReplicationAttributesToWALKey(key, context); } } @@ -2276,6 +2768,18 @@ private void appendHAGroupAttributeToWALKey(WALKey key, } } + private void appendReplicationAttributesToWALKey(WALKey key, + IndexRegionObserver.BatchMutateContext context) { + if (context == null || context.getOriginalMutations().isEmpty()) { + return; + } + Map replicationAttributes = + MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0)); + for (Map.Entry e : replicationAttributes.entrySet()) { + IndexRegionObserver.appendToWALKey(key, e.getKey(), e.getValue()); + } + } + /** * 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 @@ -2895,70 +3399,48 @@ public static boolean isAtomicOperationComplete(OperationStatus status) { private void replicateMutations(RegionCoprocessorEnvironment env, MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) throws IOException { - - if (!this.shouldReplicate) { + if (!shouldReplicate || ignoreSyncReplicationForTesting) { return; } - if (ignoreSyncReplicationForTesting) { + if (!context.logGroup.isPresent()) { return; } - - Optional logGroup = getHAGroupFromBatch(env, miniBatchOp); - if (!logGroup.isPresent()) { + // Replicated batches on the standby never re-replicate. + if (context.isReplication) { return; } - ReplicationLogGroup group = logGroup.get(); - - // Record ReplicationSyncTime only when we are actually doing work (not on early-return paths). - long start = EnvironmentEdgeManager.currentTimeMillis(); - try { - List dataTableCells = new ArrayList<>(); - for (int i = 0; i < miniBatchOp.size(); i++) { - Mutation m = miniBatchOp.getOperation(i); - if (this.ignoreReplicationFilter.test(m)) { - continue; - } - // When coprocessors add cells (local index, conditional TTL, ON DUPLICATE KEY UPDATE), - // HBase merges them into the data mutation which can mix row keys and cell types. - // We stream the cells through as-is — the consumer reconstructs Put/Delete mutations - // on the row+type boundary. - appendCells(dataTableCells, m); - } - if (!dataTableCells.isEmpty()) { - group.append(this.dataTableName, -1, dataTableCells); - } - appendIndexUpdates(group, context.preIndexUpdates); - appendIndexUpdates(group, context.postIndexUpdates); - group.sync(); - } finally { - long duration = EnvironmentEdgeManager.currentTimeMillis() - start; - metricSource.updateReplicationSyncTime(this.dataTableName, duration); - } - } - - private void appendIndexUpdates(ReplicationLogGroup group, - ListMultimap updates) throws IOException { - if (updates == null) { + if (context.getOriginalMutations().isEmpty()) { return; } - for (Map.Entry> entry : updates.asMap() - .entrySet()) { - List cells = new ArrayList<>(); - for (Mutation m : entry.getValue()) { - if (this.ignoreReplicationFilter.test(m)) { + List flattened = new ArrayList<>(); + if (context.replicationCellsByRow != null && !context.replicationCellsByRow.isEmpty()) { + // Indexed path: cells were captured in the PRE phase right after setTimestamps, with the + // per-row pre-image sidecar cells appended. Ship the captured stream as-is. + for (List bucket : context.replicationCellsByRow.values()) { + flattened.addAll(bucket); + } + } else { + // No-index path: preBatchMutateWithExceptions short-circuits at the rowsToLock.isEmpty() + // early-exit before captureReplicationCells runs. setTimestamps is a no-op for such tables, + // so the cells still carry LATEST_TIMESTAMP in PRE; HBase finalizes them only after + // preBatchMutate returns. Read the now-final cells directly from miniBatchOp here in POST. + // There are no indexes (so no pre-image cells) and no CP-injected mutations (those come from + // atomic/ON-DUP handling, which would have populated rowsToLock and skipped the early-exit). + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (ignoreReplicationFilter.test(m)) { continue; } - appendCells(cells, m); + flattened.addAll(MutationCellGrouper.flattenCells(m)); } - if (!cells.isEmpty()) { - group.append(entry.getKey().getTableName(), -1, cells); + if (flattened.isEmpty()) { + return; } } - } - - private static void appendCells(List bucket, Mutation m) { - for (List familyCells : m.getFamilyCellMap().values()) { - bucket.addAll(familyCells); - } + Map replicationAttributes = + MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0)); + ReplicationLogGroup logGroup = context.logGroup.get(); + logGroup.append(dataTableName, -1, flattened, replicationAttributes); + logGroup.sync(); } } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java index 53a86447342..b512016c823 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/builder/IndexBuildManager.java @@ -33,6 +33,7 @@ import org.apache.phoenix.coprocessorclient.BaseScannerRegionObserverConstants.ReplayWrite; import org.apache.phoenix.hbase.index.covered.IndexMetaData; import org.apache.phoenix.hbase.index.covered.data.CachedLocalTable; +import org.apache.phoenix.hbase.index.covered.data.LocalHBaseState; import org.apache.phoenix.hbase.index.table.HTableInterfaceReference; import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; import org.apache.phoenix.index.PhoenixIndexMetaData; @@ -92,14 +93,28 @@ public void getIndexUpdates( ListMultimap> indexUpdates, MiniBatchOperationInProgress miniBatchOp, Collection mutations, IndexMetaData indexMetaData) throws Throwable { - // notify the delegate that we have started processing a batch - this.delegate.batchStarted(miniBatchOp, indexMetaData); CachedLocalTable cachedLocalTable = CachedLocalTable.build(mutations, (PhoenixIndexMetaData) indexMetaData, this.regionCoprocessorEnvironment.getRegion()); + getIndexUpdates(indexUpdates, miniBatchOp, mutations, indexMetaData, cachedLocalTable); + } + + /** + * Variant of + * {@link #getIndexUpdates(ListMultimap, MiniBatchOperationInProgress, Collection, IndexMetaData)} + * that uses a caller-supplied {@link LocalHBaseState} for prior-row-state lookups instead of + * building a region-scanning {@link CachedLocalTable}. Used on the standby replay path, where + * prior state comes from the per-batch pre-image rather than the (not-yet-written) region. + */ + public void getIndexUpdates( + ListMultimap> indexUpdates, + MiniBatchOperationInProgress miniBatchOp, Collection mutations, + IndexMetaData indexMetaData, LocalHBaseState localHBaseState) throws Throwable { + // notify the delegate that we have started processing a batch + this.delegate.batchStarted(miniBatchOp, indexMetaData); // Avoid the Object overhead of the executor when it's not actually parallelizing anything. for (Mutation m : mutations) { Collection> updates = - delegate.getIndexUpdate(m, indexMetaData, cachedLocalTable); + delegate.getIndexUpdate(m, indexMetaData, localHBaseState); for (Pair update : updates) { indexUpdates.put(new HTableInterfaceReference(new ImmutableBytesPtr(update.getSecond())), new Pair<>(update.getFirst(), m.getRow())); diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java new file mode 100644 index 00000000000..270acc8bce8 --- /dev/null +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java @@ -0,0 +1,91 @@ +/* + * 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.hbase.index.covered.data; + +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.phoenix.hbase.index.IndexRegionObserver.RowTsKey; +import org.apache.phoenix.hbase.index.covered.update.ColumnReference; +import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; +import org.apache.phoenix.util.IndexUtil; + +import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions; + +/** + * Standby-side {@link LocalHBaseState} that serves the local-index builder's prior-row-state lookup + * from the per-batch pre-image the active shipped, instead of scanning the data-table region (which + * {@link CachedLocalTable} does on the active). + *

+ * On the standby the replication reader can concatenate several active-side batches into one replay + * batch, so a row recurs at several active timestamps and the region does not yet hold the + * intermediate state each batch saw. The active shipped one pre-image per row per batch, which is + * exactly the prior row state that batch built its local index against. We therefore key the lookup + * by {@code (row, ts)} — recovered from the mutation's row and max cell timestamp — and return that + * group's own pre-image cells. Each group's build is thus an independent reproduction of the + * corresponding active {@code preBatchMutate}; no chaining across groups is needed. + *

+ * A {@code null} cell list is the "active saw an empty row" sentinel and is the documented + * {@link LocalHBaseState#getCurrentRowState} return for "no prior row". A key that is absent + * entirely (never populated) is a contract violation and throws, so the sentinel stays unambiguous. + */ +public class PreImageLocalTable implements LocalHBaseState { + + /** Distinct marker for "key absent", so a stored null (empty-row sentinel) is not mistaken. */ + private static final List ABSENT = Collections.emptyList(); + + private final Map> preImageCellsByRowTs; + + public PreImageLocalTable(Map> preImageCellsByRowTs) { + this.preImageCellsByRowTs = + Preconditions.checkNotNull(preImageCellsByRowTs, "preImageCellsByRowTs must not be null"); + } + + /** + * {@inheritDoc} + *

+ * {@code toCover} and {@code ignoreNewerMutations} are ignored: we already hold the exact + * per-group prior-row snapshot the active built against, so there is nothing to narrow by column + * or filter by timestamp. The {@code (row, ts)} key is derived from the mutation the same way + * {@code IndexRegionObserver.buildReplicatedRowGroups} keys the map that populated this table. + */ + @Override + public List getCurrentRowState(Mutation mutation, + Collection toCover, boolean ignoreNewerMutations) + throws IOException { + RowTsKey key = + new RowTsKey(new ImmutableBytesPtr(mutation.getRow()), IndexUtil.getMaxTimestamp(mutation)); + // A stored null is the "active saw an empty row" sentinel, so a null value cannot be told from + // an absent key via get() alone; look up once against a distinct ABSENT marker instead. A true + // miss means the (row, ts) derivation drifted from the populating side + // (buildReplayLocalIndexInputs); fail loud rather than return null, which the builder would + // read + // as the empty-row sentinel and silently regenerate the index against an empty prior state. + List cells = preImageCellsByRowTs.getOrDefault(key, ABSENT); + if (cells == ABSENT) { + throw new DoNotRetryIOException( + "No pre-image for replayed local-index row; (row, ts) key not populated: " + key); + } + return cells; + } +} 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 d058bd192ab..a2c6e6d566d 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 @@ -19,12 +19,22 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.wal.WALEdit; +import org.apache.phoenix.execute.MutationState; +import org.apache.phoenix.hbase.index.IndexRegionObserver; +import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; +import org.apache.phoenix.index.PhoenixIndexCodec; /** * Groups a flat cell stream into Put/Delete mutations, mirroring the algorithm HBase's @@ -48,6 +58,118 @@ private static boolean isNewRowOrType(Cell previousCell, Cell cell) { || !CellUtil.matchingRows(previousCell, cell); } + /** + * Flatten a mutation's family cell map into a single ordered cell list, preserving family + * iteration order (typically TreeMap-ordered). + */ + public static List flattenCells(Mutation mutation) { + List body = new ArrayList<>(); + for (List familyCells : mutation.getFamilyCellMap().values()) { + body.addAll(familyCells); + } + return body; + } + + /** + * Extract the well-known replication attributes + * ({@link ReplicationLogGroup#REPLICATION_ATTR_KEYS}) from the mutation. Returns an empty map if + * the mutation has no attributes or none match. + *

+ * {@link PhoenixIndexCodec#INDEX_UUID}, when present, is always written as an empty byte array + * regardless of its value on the mutation. A non-empty UUID is a server-cache key scoped to the + * active cluster's region servers and is meaningless on the standby (it would fail to resolve + * with INDEX_METADATA_NOT_FOUND). An empty UUID forces the standby down the server-PTable + * resolution path, which rebuilds index maintainers from the schema/table/tenant attributes + * carried in this same envelope. + */ + public static Map extractReplicationAttributes(Mutation mutation) { + Map mutationAttrs = mutation.getAttributesMap(); + if (mutationAttrs == null || mutationAttrs.isEmpty()) { + return Collections.emptyMap(); + } + Map envelope = new HashMap<>(); + for (String key : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { + byte[] v = mutationAttrs.get(key); + if (v != null) { + envelope.put(key, + PhoenixIndexCodec.INDEX_UUID.equals(key) ? HConstants.EMPTY_BYTE_ARRAY : v); + } + } + return envelope; + } + + /** + * Build the flat cell stream the active ships for a batch of replicated mutations: each + * mutation's data cells in family order, followed by one METAFAMILY pre-image cell carrying + * {@code preImage}'s row state. This is the inverse of {@link #reconstructMutations}; replaying + * its output through that method yields back the mutations with their + * {@link IndexRegionObserver#PRE_IMAGE} attribute attached. A {@code null} {@code preImage} + * encodes the empty-row sentinel. The pre-image cell is keyed by each mutation's own row, + * mirroring the active's per-row pre-image capture. + */ + public static List buildReplicatedCells(List mutations, Put preImage) + throws IOException { + List cells = new ArrayList<>(); + for (Mutation m : mutations) { + cells.addAll(flattenCells(m)); + cells.add(IndexRegionObserver.buildPreImageCell(m.getRow(), preImage)); + } + return cells; + } + + /** + * Build the replication attribute envelope that rides alongside a {@link #buildReplicatedCells} + * cell stream: an empty {@link PhoenixIndexCodec#INDEX_UUID} (forcing the standby down the + * server-PTable resolution path, see {@link #extractReplicationAttributes}) plus the schema and + * logical table names the standby uses to rebuild index maintainers. + */ + public static Map buildReplicationAttributes(String schemaName, + String logicalTableName) { + Map attrs = new HashMap<>(); + attrs.put(PhoenixIndexCodec.INDEX_UUID, HConstants.EMPTY_BYTE_ARRAY); + attrs.put(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes(schemaName)); + attrs.put(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + Bytes.toBytes(logicalTableName)); + return attrs; + } + + /** + * Walk the record body, peeling off METAFAMILY pre-image cells (one per row) into a row-keyed + * bucket and grouping the remaining data cells into Put/Delete mutations. Each result mutation is + * stamped with the replication attributes and the generic + * {@link IndexRegionObserver#REPLICATED_MUTATION} marker. When a pre-image entry exists for its + * row, the pre-image bytes are also attached as {@link IndexRegionObserver#PRE_IMAGE}. + */ + public static List reconstructMutations(Iterable cells, + Map replicationAttrs) throws IOException { + Map preImages = new HashMap<>(); + List dataCells = new ArrayList<>(); + for (Cell c : cells) { + if ( + CellUtil.matchingFamily(c, WALEdit.METAFAMILY) + && CellUtil.matchingQualifier(c, IndexRegionObserver.PRE_IMAGE_WAL_QUALIFIER) + ) { + preImages.put(new ImmutableBytesPtr(CellUtil.cloneRow(c)), CellUtil.cloneValue(c)); + } else { + dataCells.add(c); + } + } + List mutations = splitCellsIntoMutations(dataCells); + for (Mutation m : mutations) { + if (replicationAttrs != null) { + for (Map.Entry e : replicationAttrs.entrySet()) { + m.setAttribute(e.getKey(), e.getValue()); + } + } + m.setAttribute(IndexRegionObserver.REPLICATED_MUTATION, HConstants.EMPTY_BYTE_ARRAY); + byte[] preImageBytes = preImages.get(new ImmutableBytesPtr(m.getRow())); + if (preImageBytes != null) { + m.setAttribute(IndexRegionObserver.PRE_IMAGE, preImageBytes); + } + } + return mutations; + } + /** Group a cell stream into Put/Delete mutations using the row+type boundary algorithm. */ public static List splitCellsIntoMutations(Iterable cells) throws IOException { List result = new ArrayList<>(); diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java index 4dd4eb78257..9621966ed1e 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLog.java @@ -310,7 +310,7 @@ private void replayCurrentBatch() throws IOException { LOG.info("Replaying {} unsynced records into new writer {}", currentBatch.size(), currentWriter); for (Record r : currentBatch) { - currentWriter.append(r.tableName, r.commitId, r.cells); + currentWriter.append(r.tableName, r.commitId, r.cells, r.attributes); } } @@ -350,7 +350,7 @@ private void apply(Action action) throws IOException { protected void append(Record r) throws IOException { final boolean[] blockSynced = { false }; apply(writer -> { - blockSynced[0] = writer.append(r.tableName, r.commitId, r.cells); + blockSynced[0] = writer.append(r.tableName, r.commitId, r.cells, r.attributes); }); // Add to current batch only after we succeed at appending currentBatch.add(r); diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java index 1065cd19915..6329c4b317e 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java @@ -42,6 +42,8 @@ import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.EnumSet; import java.util.List; import java.util.Map; @@ -62,6 +64,8 @@ import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.Mutation; +import org.apache.phoenix.execute.MutationState; +import org.apache.phoenix.index.PhoenixIndexCodec; import org.apache.phoenix.jdbc.HAGroupStoreManager; import org.apache.phoenix.jdbc.HAGroupStoreRecord; import org.apache.phoenix.jdbc.HAGroupStoreRecord.HAGroupState; @@ -71,6 +75,7 @@ import org.slf4j.LoggerFactory; import org.apache.phoenix.thirdparty.com.google.common.annotations.VisibleForTesting; +import org.apache.phoenix.thirdparty.com.google.common.base.Preconditions; import org.apache.phoenix.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder; import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableMap; @@ -181,6 +186,23 @@ public class ReplicationLogGroup { "phoenix.replication.log.group.shutdown.timeout.ms"; public static final long DEFAULT_REPLICATION_LOG_GROUP_SHUTDOWN_TIMEOUT_MS = 30_000L; + /** + * Batch-uniform mutation attribute keys that are carried as the envelope of a replication log + * record (and mirrored on the WAL key for the WAL-restore path). These attributes enable the + * standby cluster's IndexRegionObserver to generate index mutations from replicated data + * mutations. + *

+ * {@code REPLICATED_MUTATION} (the generic "originated from replication" marker) and + * {@code PRE_IMAGE} (per-row primary-side pre-image bytes) are intentionally NOT in this list. + * Both are reader-synthesized: the standby stamps {@code REPLICATED_MUTATION} on every + * reconstructed mutation, and {@code PRE_IMAGE} on those whose row had a pre-image cell. + */ + public static final List REPLICATION_ATTR_KEYS = + Collections.unmodifiableList(Arrays.asList(PhoenixIndexCodec.INDEX_UUID, + MutationState.MutationMetadataType.SCHEMA_NAME.toString(), + MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + MutationState.MutationMetadataType.TENANT_ID.toString())); + public static final String STANDBY_DIR = "in"; public static final String FALLBACK_DIR = "out"; @@ -331,18 +353,21 @@ public void setValues(int type, Record record, CompletableFuture syncFutur } /** - * Append payload carried through the ring buffer. Always carries a flat {@link List} of - * {@link Cell}s; the per-mutation public API extracts the cells before publishing. + * Disruptor payload for one append. Carries a flat cell stream plus the envelope attributes that + * apply uniformly to every reconstructed mutation. */ protected static class Record { public final String tableName; public final long commitId; public final List cells; + public final Map attributes; - public Record(String tableName, long commitId, List cells) { - this.tableName = tableName; + public Record(String tableName, long commitId, List cells, + Map attributes) { + this.tableName = Preconditions.checkNotNull(tableName, "tableName must not be null"); this.commitId = commitId; - this.cells = cells; + this.cells = Preconditions.checkNotNull(cells, "cells must not be null"); + this.attributes = Preconditions.checkNotNull(attributes, "attributes must not be null"); } } @@ -557,37 +582,40 @@ public void append(String tableName, long commitId, Mutation mutation) throws IO if (LOG.isTraceEnabled()) { LOG.trace("Append: table={}, commitId={}, mutation={}", tableName, commitId, mutation); } - List cells = new ArrayList<>(); - for (List familyCells : mutation.getFamilyCellMap().values()) { - cells.addAll(familyCells); - } + List cells = MutationCellGrouper.flattenCells(mutation); if (cells.isEmpty()) { throw new IllegalArgumentException("Cannot append a mutation with no cells"); } - publishDataEvent(new Record(tableName, commitId, cells)); + publish(new Record(tableName, commitId, cells, + MutationCellGrouper.extractReplicationAttributes(mutation))); } /** - * Append a coalesced batch of cells as a single record on the log. The cells must already be - * grouped by row+type so consumers can reconstruct Put/Delete mutations on the row+type boundary - * (see {@link MutationCellGrouper}). Behaves identically to - * {@link #append(String, long, Mutation)} with respect to backpressure and fail-stop. - * @param tableName The HBase table name shared by every cell in {@code cells}. - * @param commitId The commit identifier (e.g., SCN) for the batch. - * @param cells The flat ordered cell stream for one batch on one table. - * @throws IOException If the writer is closed or the ring buffer is full. + * Append a batch of cells to the log as a single record. Like + * {@link #append(String, long, Mutation)}, this method is non-blocking and returns quickly unless + * the ring buffer is full; the actual write happens asynchronously and is durable only after a + * subsequent {@link #sync()}. + * @param tableName The name of the HBase table the cells apply to. + * @param commitId The commit identifier (e.g., SCN) associated with the batch. + * @param cells The flat ordered cell stream forming the record body. Mutation reconstruction + * (grouping by row+type) is the consumer's responsibility. + * @param attributes Record-level attributes (envelope) that apply uniformly to every mutation + * reconstructed from {@code cells}. + * @throws IOException If the writer is closed or if the ring buffer is full. */ - public void append(String tableName, long commitId, List cells) throws IOException { + public void append(String tableName, long commitId, List cells, + Map attributes) throws IOException { if (cells == null || cells.isEmpty()) { throw new IllegalArgumentException("Cannot append a record with no cells"); } if (LOG.isTraceEnabled()) { - LOG.trace("Append: table={}, commitId={}, cells={}", tableName, commitId, cells.size()); + LOG.trace("Append: table={}, commitId={}, cellCount={}, attrCount={}", tableName, commitId, + cells.size(), attributes.size()); } - publishDataEvent(new Record(tableName, commitId, cells)); + publish(new Record(tableName, commitId, cells, attributes)); } - private void publishDataEvent(Record record) throws IOException { + private void publish(Record record) throws IOException { if (isClosed()) { throw new IOException("Closed"); } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFile.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFile.java index 8e3e2ea25f2..7c236c7f2f1 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFile.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFile.java @@ -378,6 +378,20 @@ interface Writer extends Closeable { */ boolean append(String tableName, long commitId, List cells) throws IOException; + /** + * Appends a batch of cells to the log file as a single record. The log record may be buffered + * internally. Mutation reconstruction (grouping by row+type) happens on the consumer side. + * @param tableName The HBase table name + * @param commitId The commit identifier + * @param cells The flat ordered cell stream forming the record body. + * @param attributes Record-level attributes (envelope) that apply uniformly to every mutation + * reconstructed from {@code cells}. + * @return true if an implicit sync happened (block full), false if buffered only + * @throws IOException if an I/O error occurs during append. + */ + boolean append(String tableName, long commitId, List cells, + Map attributes) throws IOException; + /** * Flushes any buffered data to the underlying storage and ensures it is durable (e.g., by * calling hsync on the FSDataOutputStream). This guarantees that records appended before the diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileRecord.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileRecord.java index e9c5b807b07..dee0ae66196 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileRecord.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileRecord.java @@ -18,7 +18,6 @@ package org.apache.phoenix.replication.log; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; @@ -89,15 +88,7 @@ public LogFile.Record setAttributes(Map attributes) { @Override public List getMutations() throws IOException { - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - if (!attributes.isEmpty()) { - for (Mutation m : result) { - for (Map.Entry e : attributes.entrySet()) { - m.setAttribute(e.getKey(), e.getValue()); - } - } - } - return result; + return MutationCellGrouper.reconstructMutations(cells, attributes); } @Override @@ -112,12 +103,8 @@ public Mutation getMutation() throws IOException { @Override public LogFile.Record setMutation(Mutation mutation) { - List body = new ArrayList<>(); - for (List familyCells : mutation.getFamilyCellMap().values()) { - body.addAll(familyCells); - } - this.cells = body; - this.attributes = Collections.emptyMap(); + this.cells = MutationCellGrouper.flattenCells(mutation); + this.attributes = MutationCellGrouper.extractReplicationAttributes(mutation); return this; } diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileWriter.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileWriter.java index df3d4288648..3404fe0baa2 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileWriter.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileWriter.java @@ -19,7 +19,9 @@ import java.io.FileNotFoundException; import java.io.IOException; +import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FileSystem; @@ -81,11 +83,17 @@ public boolean isClosed() { @Override public boolean append(String tableName, long commitId, List cells) throws IOException { + return append(tableName, commitId, cells, Collections.emptyMap()); + } + + @Override + public boolean append(String tableName, long commitId, List cells, + Map attributes) throws IOException { if (isClosed()) { throw new IOException("Writer has been closed"); } - return writer.append( - new LogFileRecord().setHBaseTableName(tableName).setCommitId(commitId).setCells(cells)); + return writer.append(new LogFileRecord().setHBaseTableName(tableName).setCommitId(commitId) + .setCells(cells).setAttributes(attributes)); } @Override diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogProcessor.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogProcessor.java index d9e488394cb..414bd1bb63a 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogProcessor.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/reader/ReplicationLogProcessor.java @@ -33,6 +33,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.LeaseRecoverable; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.TableName; @@ -246,22 +247,25 @@ public void processLogFile(FileSystem fs, Path filePath) throws IOException { for (LogFile.Record record : logFileReader) { final TableName tableName = TableName.valueOf(record.getHBaseTableName()); - // A record may reconstruct into multiple mutations. Batches split on the mutation - // boundary, so a single record's mutations can span two processReplicationLogBatch - // invocations -- do not assume per-record atomicity here. + // A record is a batch: its body is a flat cell stream that may contain METAFAMILY + // pre-image cells. getMutations() peels those pre-image cells off, stamps every + // reconstructed mutation with the generic REPLICATED_MUTATION marker, and attaches the + // per-row pre-image bytes as PRE_IMAGE on the owning row's mutation. The standby IRO + // uses PRE_IMAGE to populate dataRowStates without scanning the data table. + // All mutations of a record are added to the current batch together and the size check + // is deferred to the record boundary, so a record (and every (row, ts) group within it) + // is never split across two batches. for (Mutation mutation : record.getMutations()) { tableToMutationsMap.computeIfAbsent(tableName, k -> new ArrayList<>()).add(mutation); currentBatchSize++; currentBatchSizeBytes += mutation.heapSize(); - - // Process when we reach either the batch count or size limit - if (currentBatchSize >= getBatchSize() || currentBatchSizeBytes >= getBatchSizeBytes()) { - processReplicationLogBatch(tableToMutationsMap); - totalProcessed += currentBatchSize; - tableToMutationsMap.clear(); - currentBatchSize = 0; - currentBatchSizeBytes = 0; - } + } + if (currentBatchSize >= getBatchSize() || currentBatchSizeBytes >= getBatchSizeBytes()) { + processReplicationLogBatch(tableToMutationsMap); + totalProcessed += currentBatchSize; + tableToMutationsMap.clear(); + currentBatchSize = 0; + currentBatchSizeBytes = 0; } } @@ -402,6 +406,14 @@ protected void processReplicationLogBatch(Map> tableMu } attempt++; getMetrics().incrementFailedBatchCount(); + // A DoNotRetryIOException is deterministic (e.g. a schema/contract violation): retrying the + // same mutations will fail identically. Honor that contract as HBase's own retrying caller + // does and stop immediately rather than burning batchRetryCount attempts with backoff sleeps. + if (isNonRetryable(lastError)) { + LOG.error("Not retrying batch operations; failure is non-retryable. Failed tables: {}", + currentOperations.keySet()); + break; + } // Add delay between retries (exponential backoff) if (attempt <= batchRetryCount && !currentOperations.isEmpty()) { try { @@ -425,6 +437,21 @@ protected void processReplicationLogBatch(Map> tableMu } } + /** + * Returns true if the failure is a {@link DoNotRetryIOException}, walking the cause chain because + * the batch client wraps it (as a + * {@link org.apache.hadoop.hbase.client.RetriesExhaustedException} cause) by the time it reaches + * us. + */ + private static boolean isNonRetryable(Throwable error) { + for (Throwable cur = error; cur != null; cur = cur.getCause()) { + if (cur instanceof DoNotRetryIOException) { + return true; + } + } + return false; + } + /** * Calculates the delay time for retry attempts using exponential backoff. * @param attempt The current retry attempt number (0-based) @@ -469,7 +496,7 @@ protected ApplyMutationBatchResult applyMutations(Map> // Add failed mutations to retry list failedOperations.put(tableName, tableMutationMap.get(tableName)); getMetrics().incrementFailedMutationsCount(tableMutationMap.get(tableName).size()); - LOG.debug("Failed to apply mutations for table {}: {}", tableName, e.getMessage()); + LOG.debug("Failed to apply mutations for table {}", tableName, e); lastException = e; } } diff --git a/phoenix-core/src/it/java/org/apache/phoenix/hbase/index/write/TestTrackingParallelWriterIndexCommitter.java b/phoenix-core/src/it/java/org/apache/phoenix/hbase/index/write/TestTrackingParallelWriterIndexCommitter.java index d2867d8079c..25ab06ef456 100644 --- a/phoenix-core/src/it/java/org/apache/phoenix/hbase/index/write/TestTrackingParallelWriterIndexCommitter.java +++ b/phoenix-core/src/it/java/org/apache/phoenix/hbase/index/write/TestTrackingParallelWriterIndexCommitter.java @@ -39,26 +39,25 @@ public class TestTrackingParallelWriterIndexCommitter extends TrackingParallelWr public static class TestConnectionFactory extends ServerUtil.ConnectionFactory { - private static Map connections = - new ConcurrentHashMap(); + private static Map connections = + new ConcurrentHashMap(); public static Connection getConnection(final ServerUtil.ConnectionType connectionType, final RegionCoprocessorEnvironment env) { final String key = String.format("%s-%s", env.getServerName(), connectionType.name().toLowerCase()); LOGGER.info("Connecting to {}", key); - return connections.computeIfAbsent(connectionType, - new Function() { - @Override - public Connection apply(ServerUtil.ConnectionType t) { - try { - return env.createConnection( - getTypeSpecificConfiguration(connectionType, env.getConfiguration())); - } catch (IOException e) { - throw new RuntimeException(e); - } + return connections.computeIfAbsent(key, new Function() { + @Override + public Connection apply(String k) { + try { + return env.createConnection( + getTypeSpecificConfiguration(connectionType, env.getConfiguration())); + } catch (IOException e) { + throw new RuntimeException(e); } - }); + } + }); } public static void shutdown() { diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupBaseIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupBaseIT.java new file mode 100644 index 00000000000..95df33a5d33 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupBaseIT.java @@ -0,0 +1,527 @@ +/* + * 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.replication; + +import static org.apache.phoenix.hbase.index.IndexRegionObserver.PHOENIX_INDEX_CDC_MUTATION_SERIALIZE; +import static org.apache.phoenix.jdbc.HighAvailabilityGroup.PHOENIX_HA_GROUP_ATTR; +import static org.apache.phoenix.jdbc.HighAvailabilityTestingUtility.getHighAvailibilityGroup; +import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME; +import static org.apache.phoenix.query.QueryServices.SYNCHRONOUS_REPLICATION_ENABLED; +import static org.apache.phoenix.replication.ReplicationShardDirectoryManager.PHOENIX_REPLICATION_ROUND_DURATION_SECONDS_KEY; +import static org.apache.phoenix.replication.reader.ReplicationLogReplayService.PHOENIX_REPLICATION_REPLAY_ENABLED; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.DriverManager; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Properties; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.hadoop.fs.FileStatus; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.client.ConnectionFactory; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.client.RegionLocator; +import org.apache.hadoop.hbase.client.Result; +import org.apache.hadoop.hbase.client.ResultScanner; +import org.apache.hadoop.hbase.client.Scan; +import org.apache.hadoop.hbase.client.Table; +import org.apache.hadoop.hbase.regionserver.HRegionServer; +import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.VersionInfo; +import org.apache.phoenix.expression.function.PartitionIdFunction; +import org.apache.phoenix.jdbc.HABaseIT; +import org.apache.phoenix.jdbc.HighAvailabilityGroup; +import org.apache.phoenix.jdbc.HighAvailabilityPolicy; +import org.apache.phoenix.jdbc.HighAvailabilityTestingUtility; +import org.apache.phoenix.jdbc.PhoenixDriver; +import org.apache.phoenix.query.QueryConstants; +import org.apache.phoenix.replication.reader.ReplicationLogProcessor; +import org.apache.phoenix.replication.tool.LogFileAnalyzer; +import org.apache.phoenix.util.TestUtil; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Rule; +import org.junit.rules.TestName; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Shared fixture for the replication log-group ITs: cluster setup, per-test HA-group wiring, and + * the replay/verification helpers. Concrete subclasses supply their own {@code @BeforeClass} + * calling {@link #setupClusters()} so each runs exactly the tests it declares; nothing here is a + * {@code @Test}. + */ +public abstract class ReplicationLogGroupBaseIT extends HABaseIT { + protected static final Logger LOG = LoggerFactory.getLogger(ReplicationLogGroupBaseIT.class); + + @Rule + public TestName name = new TestName(); + + protected Properties clientProps = new Properties(); + protected String haGroupName; + protected HighAvailabilityGroup haGroup; + protected ReplicationLogGroup logGroup; + + /** + * Starts both clusters with the common replication test config. A subclass that needs a + * cluster-level toggle (e.g. {@code serializeCDCMutations}, read once at IRO {@code start()}) + * sets it on {@code conf1}/{@code conf2} in its own {@code @BeforeClass} before calling this. + */ + protected static void setupClusters() throws Exception { + conf1.setInt(PHOENIX_REPLICATION_ROUND_DURATION_SECONDS_KEY, 5); + // Disable replay on cluster 1 + conf1.setBoolean(PHOENIX_REPLICATION_REPLAY_ENABLED, false); + // Disable replay on cluster 2, we will explicitly replay the log files + conf2.setBoolean(PHOENIX_REPLICATION_REPLAY_ENABLED, false); + // Disable writer on cluster 2 + conf2.setBoolean(SYNCHRONOUS_REPLICATION_ENABLED, false); + CLUSTERS.start(); + DriverManager.registerDriver(PhoenixDriver.INSTANCE); + } + + @AfterClass + public static void tearDownAfterClass() throws Exception { + DriverManager.deregisterDriver(PhoenixDriver.INSTANCE); + CLUSTERS.close(); + } + + @Before + public void beforeTest() throws Exception { + LOG.info("Starting test {}", name.getMethodName()); + haGroupName = name.getMethodName(); + clientProps = HighAvailabilityTestingUtility.getHATestProperties(); + clientProps.setProperty(PHOENIX_HA_GROUP_ATTR, haGroupName); + CLUSTERS.initClusterRole(haGroupName, HighAvailabilityPolicy.FAILOVER); + haGroup = getHighAvailibilityGroup(CLUSTERS.getJdbcHAUrl(), clientProps); + LOG.info("Initialized haGroup {} with URL {}", haGroup, CLUSTERS.getJdbcHAUrl()); + logGroup = getReplicationLogGroup(); + } + + @After + public void afterTest() throws Exception { + LOG.info("Starting cleanup for test {}", name.getMethodName()); + logGroup.close(); + LOG.info("Ending cleanup for test {}", name.getMethodName()); + } + + private ReplicationLogGroup getReplicationLogGroup() throws IOException { + HRegionServer rs = CLUSTERS.getHBaseCluster1().getHBaseCluster().getRegionServer(0); + return ReplicationLogGroup.get(conf1, rs.getServerName(), haGroupName); + } + + protected Map> groupLogsByTable() throws Exception { + LogFileAnalyzer analyzer = new LogFileAnalyzer(); + // use peer cluster conf + analyzer.setConf(conf2); + Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); + LOG.info("Analyzing log files at {}", standByLogDir); + String[] args = { "--check", standByLogDir.toString() }; + assertEquals(0, analyzer.run(args)); + return analyzer.groupLogsByTable(standByLogDir.toString()); + } + + protected int getCountForTable(Map> logsByTable, String tableName) + throws Exception { + List mutations = logsByTable.get(tableName); + return mutations != null ? mutations.size() : 0; + } + + protected Map countRecordsByTable() throws Exception { + LogFileAnalyzer analyzer = new LogFileAnalyzer(); + // use peer cluster conf + analyzer.setConf(conf2); + Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); + return analyzer.countRecordsByTable(standByLogDir.toString()); + } + + /** + * Counts how many of the given log files actually carry records. A single RS writes one file per + * replication round and rotates at every round boundary, so a short workload leaves several empty + * round-files around the one that holds its mutations. Empty files contribute no work to replay, + * so this -- not the raw file count -- is the true measure of how many files a parallel replay + * overlaps on. Reads only record counts (no mutation expansion), so it is cheap. + */ + protected int countFilesWithRecords(List logFiles) throws Exception { + LogFileAnalyzer analyzer = new LogFileAnalyzer(); + analyzer.setConf(conf2); + int populated = 0; + for (Path logFile : logFiles) { + if (!analyzer.countRecordsByTable(logFile.toString()).isEmpty()) { + populated++; + } + } + return populated; + } + + protected void verifyReplication(Map expected) throws Exception { + // first close the logGroup + logGroup.close(); + Map> mutationsByTable = groupLogsByTable(); + dumpTableLogCount(mutationsByTable); + for (Map.Entry entry : expected.entrySet()) { + String tableName = entry.getKey(); + int expectedMutationCount = entry.getValue(); + List mutations = mutationsByTable.get(tableName); + int actualMutationCount = mutations != null ? mutations.size() : 0; + try { + if (!tableName.equals(SYSTEM_CATALOG_NAME)) { + assertEquals(String.format("For table %s", tableName), expectedMutationCount, + actualMutationCount); + } else { + // special handling for syscat + assertTrue("For SYSCAT", actualMutationCount >= expectedMutationCount); + } + } catch (AssertionError e) { + // create a regular connection + try (Connection conn = DriverManager.getConnection(CLUSTERS.getJdbcUrl1(haGroup))) { + TestUtil.dumpTable(conn, TableName.valueOf(tableName)); + throw e; + } + } + } + } + + protected void dumpTableLogCount(Map> mutationsByTable) { + LOG.info("Dump table log count for test {}", name.getMethodName()); + for (Map.Entry> table : mutationsByTable.entrySet()) { + LOG.info("#Log entries for {} = {}", table.getKey(), table.getValue().size()); + } + } + + protected void moveRegionToServer(TableName tableName, ServerName sn) throws Exception { + HBaseTestingUtility util = CLUSTERS.getHBaseCluster1(); + try (RegionLocator locator = util.getConnection().getRegionLocator(tableName)) { + String regEN = locator.getAllRegionLocations().get(0).getRegionInfo().getEncodedName(); + while (!sn.equals(locator.getAllRegionLocations().get(0).getServerName())) { + LOG.info("Moving region {} of table {} to server {}", regEN, tableName, sn); + util.getAdmin().move(Bytes.toBytes(regEN), sn); + Thread.sleep(100); + } + LOG.info("Moved region {} of table {} to server {}", regEN, tableName, sn); + } + } + + protected void replayAndVerifyAcrossClusters(List ddlStatements, String... tablesToVerify) + throws Exception { + replayAndVerifyAcrossClusters(1, ddlStatements, tablesToVerify); + } + + /** + * Creates the schema on cluster 2, replays the replication log, and asserts cross-cluster cell + * equality for each named table. With {@code parallelism == 1} the log files are replayed + * sequentially in the calling thread; with {@code parallelism > 1} they are sharded round-robin + * across that many threads, all sharing one {@link ReplicationLogProcessor}. This simulates + * multiple region servers draining shard files in parallel within the same replay round, driving + * overlapping same-row batches through the standby IRO concurrently. Replay order does not affect + * the result: each {@code (row, ts)} group is an independent reproduction of the active's + * pre-batch state. + */ + protected void replayAndVerifyAcrossClusters(int parallelism, List ddlStatements, + String... tablesToVerify) throws Exception { + Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); + + // Quiesce the log group before reading: this stops rotation and synchronously closes the + // writers, so every file has a durable trailer before any reader recovers its lease. Replaying + // against a still-open group lets lease recovery fence a writer mid-rotation, costing it its + // trailer. close() is idempotent, so this is a no-op when verifyReplication already closed it. + logGroup.close(); + + // Create the same schema on cluster 2 + try (Connection conn2 = CLUSTERS.getCluster2Connection(haGroup)) { + for (String ddl : ddlStatements) { + conn2.createStatement().execute(ddl); + } + conn2.commit(); + } + + // Replay replication log on cluster 2 + FileSystem fs = standByLogDir.getFileSystem(conf2); + List logFiles = findLogFiles(standByLogDir, fs); + assertTrue("Should have at least one log file", !logFiles.isEmpty()); + ReplicationLogProcessor processor = ReplicationLogProcessor.get(conf2, haGroupName); + try { + if (parallelism <= 1) { + for (Path logFile : logFiles) { + LOG.info("Replaying log file: {}", logFile); + processor.processLogFile(fs, logFile); + } + } else { + // A single populated file would silently degrade parallel replay to the sequential case + // (empty round-boundary files do no work), so a test that asked for concurrency must have + // mutations spread across more than one file -- i.e. the workload spanned several rounds. + // Fail loudly rather than pass with weaker coverage. + int populated = countFilesWithRecords(logFiles); + assertTrue("Parallel replay requested but only " + populated + " of " + logFiles.size() + + " log file(s) carry records; the workload must span multiple rounds", populated > 1); + replayLogFilesInParallel(processor, fs, logFiles, parallelism); + } + } finally { + processor.close(); + } + + // Verify tables match across clusters at the HBase cell level + for (String table : tablesToVerify) { + assertTablesEqualAcrossClusters(table); + } + } + + /** + * Replays the given log files concurrently across {@code parallelism} threads sharing one + * processor ({@code processLogFile} is stateless apart from the lazily-built, double-checked + * {@code AsyncConnection}, so concurrent calls are safe). Surfaces the first thread's failure as + * an {@link AssertionError} so a replay error fails the test rather than being swallowed. + */ + private void replayLogFilesInParallel(ReplicationLogProcessor processor, FileSystem fs, + List logFiles, int parallelism) throws Exception { + int threads = Math.min(parallelism, logFiles.size()); + CountDownLatch doneSignal = new CountDownLatch(threads); + AtomicReference firstError = new AtomicReference<>(); + for (int t = 0; t < threads; t++) { + final int offset = t; + final int stride = threads; + Thread worker = new Thread(() -> { + try { + for (int i = offset; i < logFiles.size(); i += stride) { + Path logFile = logFiles.get(i); + LOG.info("Replaying log file (thread {}): {}", offset, logFile); + processor.processLogFile(fs, logFile); + } + } catch (Throwable e) { + firstError.compareAndSet(null, e); + } finally { + doneSignal.countDown(); + } + }, "replay-worker-" + t); + worker.start(); + } + assertTrue("Ran out of time waiting for parallel replay", + doneSignal.await(120, TimeUnit.SECONDS)); + if (firstError.get() != null) { + throw new AssertionError("A parallel replay thread failed", firstError.get()); + } + } + + /** + * True for HBase 2.4.18+, 2.5.9+, and 2.6.0+, where atomic upsert with {@code RETURNING *} + * correctly projects the returned row. Mirrors {@code OnDuplicateKey2IT}. + */ + protected boolean isSetCorrectResultEnabledOnHBase() { + String hbaseVersion = VersionInfo.getVersion(); + String[] versionArr = hbaseVersion.split("\\."); + int majorVersion = Integer.parseInt(versionArr[0]); + int minorVersion = Integer.parseInt(versionArr[1]); + int patchVersion = Integer.parseInt(versionArr[2].split("-")[0]); + if (majorVersion != 2) { + return majorVersion > 2; + } + if (minorVersion >= 6) { + return true; + } + if (minorVersion < 4) { + return false; + } + if (minorVersion == 4) { + return patchVersion >= 18; + } + return patchVersion >= 9; + } + + protected List findLogFiles(Path dir, FileSystem fs) throws IOException { + List files = new ArrayList<>(); + findLogFilesRecursive(dir, fs, files); + return files; + } + + private void findLogFilesRecursive(Path dir, FileSystem fs, List files) throws IOException { + if (!fs.exists(dir)) { + return; + } + for (FileStatus status : fs.listStatus(dir)) { + if (status.isDirectory()) { + findLogFilesRecursive(status.getPath(), fs, files); + } else if (status.getPath().getName().endsWith(".plog")) { + files.add(status.getPath()); + } + } + } + + /** + * Compares an HBase table between the two clusters using {@link Result#compareResults}. Scans + * both tables with all versions and asserts that every row matches at the cell level. + */ + protected void assertTablesEqualAcrossClusters(String hbaseTableName) throws Exception { + TableName tn = TableName.valueOf(hbaseTableName); + try ( + org.apache.hadoop.hbase.client.Connection hconn1 = ConnectionFactory.createConnection(conf1); + org.apache.hadoop.hbase.client.Connection hconn2 = ConnectionFactory.createConnection(conf2); + Table table1 = hconn1.getTable(tn); Table table2 = hconn2.getTable(tn)) { + + Scan scan = new Scan(); + scan.readAllVersions(); + + try (ResultScanner scanner1 = table1.getScanner(scan); + ResultScanner scanner2 = table2.getScanner(scan)) { + int rowCount = 0; + while (true) { + Result r1 = scanner1.next(); + Result r2 = scanner2.next(); + if (r1 == null && r2 == null) { + break; + } + assertNotNull( + String.format("Table %s: cluster 2 has fewer rows at row %d", hbaseTableName, rowCount), + r2); + assertNotNull( + String.format("Table %s: cluster 1 has fewer rows at row %d", hbaseTableName, rowCount), + r1); + try { + Result.compareResults(r1, r2, true); + } catch (Exception e) { + LOG.error("Table {} row {} mismatch. Dumping both tables:", hbaseTableName, rowCount); + LOG.error("--- Cluster 1 ---"); + TestUtil.dumpTable(table1); + LOG.error("--- Cluster 2 ---"); + TestUtil.dumpTable(table2); + fail(String.format("Table %s row %d mismatch: %s", hbaseTableName, rowCount, + e.getMessage())); + } + rowCount++; + } + LOG.info("Table {} matches across clusters: {} rows verified", hbaseTableName, rowCount); + } + } + } + + /** + * Cross-cluster equality for a CDC index physical table. A CDC index rowkey leads with + * {@code PARTITION_ID()} = the encoded data-table region name ({@code PARTITION_ID_LENGTH} + * bytes), which is region-local and so differs by design between the active and the standby. The + * standby regenerates the rowkey with its own partition_id, so a byte-equal compare on the full + * rowkey (as {@link #assertTablesEqualAcrossClusters} does) would always fail. Everything else is + * identical: the rowkey suffix (ROW_TIMESTAMP + data PK) and every cell (the index is uncovered, + * so both sides write the empty cell {@code UNVERIFIED} and there is no post phase to flip it). + * This compares the rowkey suffix after the partition_id and all cell content except the row. + */ + protected void assertCDCIndexEqualAcrossClusters(String hbaseTableName) throws Exception { + final int pidLen = PartitionIdFunction.PARTITION_ID_LENGTH; + TableName tn = TableName.valueOf(hbaseTableName); + try ( + org.apache.hadoop.hbase.client.Connection hconn1 = ConnectionFactory.createConnection(conf1); + org.apache.hadoop.hbase.client.Connection hconn2 = ConnectionFactory.createConnection(conf2); + Table table1 = hconn1.getTable(tn); Table table2 = hconn2.getTable(tn)) { + + Scan scan = new Scan(); + scan.readAllVersions(); + try (ResultScanner scanner1 = table1.getScanner(scan); + ResultScanner scanner2 = table2.getScanner(scan)) { + int rowCount = 0; + while (true) { + Result r1 = scanner1.next(); + Result r2 = scanner2.next(); + if (r1 == null && r2 == null) { + break; + } + assertNotNull(String.format("CDC index %s: cluster 2 has fewer rows at row %d", + hbaseTableName, rowCount), r2); + assertNotNull(String.format("CDC index %s: cluster 1 has fewer rows at row %d", + hbaseTableName, rowCount), r1); + + byte[] rk1 = r1.getRow(); + byte[] rk2 = r2.getRow(); + assertTrue(String.format("CDC index %s row %d: rowkey shorter than partition_id", + hbaseTableName, rowCount), rk1.length >= pidLen && rk2.length >= pidLen); + assertArrayEquals( + String.format("CDC index %s row %d: rowkey suffix after partition_id differs", + hbaseTableName, rowCount), + Arrays.copyOfRange(rk1, pidLen, rk1.length), + Arrays.copyOfRange(rk2, pidLen, rk2.length)); + + List cells1 = r1.listCells(); + List cells2 = r2.listCells(); + assertEquals( + String.format("CDC index %s row %d: cell count differs", hbaseTableName, rowCount), + cells1.size(), cells2.size()); + for (int i = 0; i < cells1.size(); i++) { + Cell c1 = cells1.get(i); + Cell c2 = cells2.get(i); + String where = + String.format("CDC index %s row %d cell %d", hbaseTableName, rowCount, i); + assertTrue(where + ": family differs", CellUtil.matchingFamily(c1, c2)); + assertTrue(where + ": qualifier differs", CellUtil.matchingQualifier(c1, c2)); + assertEquals(where + ": timestamp differs", c1.getTimestamp(), c2.getTimestamp()); + assertEquals(where + ": type differs", c1.getType(), c2.getType()); + assertTrue(where + ": value differs", CellUtil.matchingValue(c1, c2)); + } + rowCount++; + } + LOG.info("CDC index {} matches across clusters (partition_id-stripped): {} rows verified", + hbaseTableName, rowCount); + } + } + } + + /** + * Asserts the CDC index's {@code _IDX_PRE_} serialized-payload column is present iff + * {@code serializeCDCMutations} is enabled on the cluster. Scans the standby (cluster 2) copy, + * which is the regenerated one, so a present payload also proves the standby reproduced it. + */ + protected void assertCDCIndexPayloadMatchesConfig(String hbaseTableName) throws Exception { + boolean serialize = conf1.getBoolean(PHOENIX_INDEX_CDC_MUTATION_SERIALIZE, false); + TableName tn = TableName.valueOf(hbaseTableName); + int payloadCells = 0; + try ( + org.apache.hadoop.hbase.client.Connection hconn = ConnectionFactory.createConnection(conf2); + Table table = hconn.getTable(tn)) { + Scan scan = new Scan(); + scan.readAllVersions(); + try (ResultScanner scanner = table.getScanner(scan)) { + for (Result r = scanner.next(); r != null; r = scanner.next()) { + for (Cell c : r.listCells()) { + if (CellUtil.matchingQualifier(c, QueryConstants.CDC_INDEX_PRE_MUTATIONS_CQ_BYTES)) { + payloadCells++; + } + } + } + } + } + if (serialize) { + assertTrue("serializeCDCMutations=true but CDC index " + hbaseTableName + + " carries no _IDX_PRE_ payload cell on the standby", payloadCells > 0); + } else { + assertEquals("serializeCDCMutations=false but CDC index " + hbaseTableName + + " carries _IDX_PRE_ payload cells on the standby", 0, payloadCells); + } + } +} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexIT.java new file mode 100644 index 00000000000..195f9d1e2e0 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexIT.java @@ -0,0 +1,144 @@ +/* + * 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.replication; + +import static org.apache.phoenix.hbase.index.IndexRegionObserver.PHOENIX_INDEX_CDC_CONSUMER_ENABLED; +import static org.apache.phoenix.query.BaseTest.generateUniqueName; + +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.util.Arrays; +import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; +import org.apache.phoenix.jdbc.FailoverPhoenixConnection; +import org.apache.phoenix.util.CDCUtil; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +/** + * CDC index behind a CONSISTENCY=EVENTUAL secondary index. Lives in its own IT (not in + * {@link ReplicationLogGroupIT}) because the {@code serializeCDCMutations} variant is a + * cluster-level config and is exercised by the + * {@link ReplicationLogGroupEventualIndexWithSerializeCDCIT} subclass, which inherits exactly this + * one test. The base class runs it under the default {@code serializeCDCMutations=false}. + */ +@Category(NeedsOwnMiniClusterTest.class) +public class ReplicationLogGroupEventualIndexIT extends ReplicationLogGroupBaseIT { + + @BeforeClass + public static void doSetup() throws Exception { + setupEventualIndexClusters(); + } + + /** + * Disables the IndexCDCConsumer on both clusters, then starts them. CDC-index regeneration is + * verified directly and the consumer's downstream secondary-index convergence is out of scope, so + * it stays off. The {@code serializeCDCMutations} subclass calls this after setting its own + * toggle so the consumer-disable lives in one place. + */ + protected static void setupEventualIndexClusters() throws Exception { + conf1.setBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, false); + conf2.setBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, false); + setupClusters(); + } + + /** + * An eventually-consistent secondary index auto-creates a CDC index behind it ({@code CDC_ + * + + * } -> {@code PHOENIX_CDC_INDEX_CDC_ + * +
+ * }, MetaDataClient.java:2473). That CDC index is STRONG/uncovered and written inline on the data + * path, so the standby regenerates it from the data record + per-(row,ts) pre-image with its own + * partition_id, exactly like a plain CDC index. This verifies the CDC index table matches across + * clusters (modulo partition_id). The eventual secondary index table itself is written only by + * the (here-disabled) IndexCDCConsumer, so it stays empty on both clusters and is not compared. + * The serialized downstream-index payload column is present iff {@code serializeCDCMutations} is + * enabled (asserted via {@link #assertCDCIndexPayloadMatchesConfig}); the {@code serialize=true} + * variant is exercised by {@link ReplicationLogGroupEventualIndexWithSerializeCDCIT}. + */ + @Test + public void testEventualIndexCDCTable() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + final String cdcIndexName = CDCUtil.getCDCIndexName("CDC_" + tableName); + String createTableDdl = String.format( + "create table if not exists %s (pk integer not null " + "primary key, a varchar, b varchar)", + tableName); + String createIndexDdl = + String.format("create index if not exists %s on %s (a) include (b) consistency=eventual", + indexName, tableName); + // CONSISTENCY=EVENTUAL auto-creates this CDC object (CDC_) and its physical CDC + // index. We must issue the CREATE CDC explicitly, before the index, on BOTH clusters. The two + // mini-clusters share one JVM metadata cache: cluster 1's CREATE INDEX registers the index + // PTable in that cache, so when the same DDL replays on cluster 2 the index appears to already + // exist and createIndex returns early (table == null) WITHOUT reaching the nested + // createCDCForEventuallyConsistentIndex (MetaDataClient.java:1915-1925). The CDC index physical + // region would then never be created on cluster 2 and the replayed index writes fail with + // "Cannot get replica 0 location". Issuing CREATE CDC IF NOT EXISTS directly runs CREATE + // UNCOVERED INDEX -> ensureTableCreated (which checks the per-cluster HBase admin, not the + // shared cache), so the physical CDC index region is created on each cluster. + String createCdcDdl = + String.format("create cdc if not exists \"CDC_%s\" on %s", tableName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createCdcDdl); + conn.createStatement().execute(createIndexDdl); + conn.commit(); + + PreparedStatement upsert = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?)"); + for (int i = 0; i < 10; ++i) { + upsert.setInt(1, i); + upsert.setString(2, "a_" + i); + upsert.setString(3, "b_" + i); + upsert.executeUpdate(); + conn.commit(); + } + + PreparedStatement update = + conn.prepareStatement("upsert into " + tableName + " (pk, a) VALUES(?, ?)"); + for (int i = 0; i < 5; ++i) { + update.setInt(1, i); + update.setString(2, "a2_" + i); + update.executeUpdate(); + } + conn.commit(); + + PreparedStatement delete = + conn.prepareStatement("delete from " + tableName + " where pk = ?"); + for (int i = 5; i < 8; ++i) { + delete.setInt(1, i); + delete.executeUpdate(); + } + conn.commit(); + + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createCdcDdl, createIndexDdl), + tableName); + assertCDCIndexEqualAcrossClusters(cdcIndexName); + // The cross-cluster compare above is symmetric, so it passes whether or not the serialized + // downstream-index payload is present. Assert it positively against the live config so a + // serialize=true run that failed to propagate would fail loudly rather than pass + // degenerately. + assertCDCIndexPayloadMatchesConfig(cdcIndexName); + } + } +} diff --git a/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexWithSerializeCDCIT.java b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexWithSerializeCDCIT.java new file mode 100644 index 00000000000..36cf9c60855 --- /dev/null +++ b/phoenix-core/src/it/java/org/apache/phoenix/replication/ReplicationLogGroupEventualIndexWithSerializeCDCIT.java @@ -0,0 +1,46 @@ +/* + * 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.replication; + +import static org.apache.phoenix.hbase.index.IndexRegionObserver.PHOENIX_INDEX_CDC_MUTATION_SERIALIZE; + +import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; +import org.junit.BeforeClass; +import org.junit.experimental.categories.Category; + +/** + * Runs the eventual-index CDC scenario with {@code serializeCDCMutations=true}: the standby + * regenerates the CDC index row including the serialized downstream-index {@code _IDX_PRE_}/ + * {@code _IDX_POST_} payload (see {@code prepareEventuallyConsistentIndexMutations}). Inherits the + * single test from {@link ReplicationLogGroupEventualIndexIT} and only flips the cluster-level + * {@code serializeCDCMutations} config; {@code assertCDCIndexPayloadMatchesConfig} then asserts the + * payload is present. + */ +@Category(NeedsOwnMiniClusterTest.class) +public class ReplicationLogGroupEventualIndexWithSerializeCDCIT + extends ReplicationLogGroupEventualIndexIT { + + @BeforeClass + public static void doSetup() throws Exception { + // serializeCDCMutations is read once at IRO start(), so set it before the clusters come up; + // setupEventualIndexClusters() handles the consumer-disable and cluster start. + conf1.setBoolean(PHOENIX_INDEX_CDC_MUTATION_SERIALIZE, true); + conf2.setBoolean(PHOENIX_INDEX_CDC_MUTATION_SERIALIZE, true); + setupEventualIndexClusters(); + } +} 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 c4dd5149b20..fc4cc9f5633 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 @@ -17,249 +17,68 @@ */ package org.apache.phoenix.replication; -import static org.apache.phoenix.jdbc.HighAvailabilityGroup.PHOENIX_HA_GROUP_ATTR; -import static org.apache.phoenix.jdbc.HighAvailabilityTestingUtility.getHighAvailibilityGroup; +import static org.apache.phoenix.hbase.index.IndexRegionObserver.PHOENIX_INDEX_CDC_CONSUMER_ENABLED; import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.SYSTEM_CATALOG_NAME; import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.SYSTEM_CHILD_LINK_NAME; import static org.apache.phoenix.query.BaseTest.generateUniqueName; -import static org.apache.phoenix.replication.ReplicationShardDirectoryManager.PHOENIX_REPLICATION_ROUND_DURATION_SECONDS_KEY; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; -import java.io.IOException; import java.sql.Connection; import java.sql.DriverManager; import java.sql.PreparedStatement; -import java.util.ArrayList; +import java.sql.ResultSet; +import java.sql.Statement; import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Properties; +import java.util.Random; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; -import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.CellUtil; import org.apache.hadoop.hbase.HBaseTestingUtility; import org.apache.hadoop.hbase.MiniHBaseCluster; import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.TableName; -import org.apache.hadoop.hbase.client.ConnectionFactory; import org.apache.hadoop.hbase.client.Mutation; -import org.apache.hadoop.hbase.client.RegionLocator; -import org.apache.hadoop.hbase.client.Result; -import org.apache.hadoop.hbase.client.ResultScanner; -import org.apache.hadoop.hbase.client.Scan; -import org.apache.hadoop.hbase.client.Table; -import org.apache.hadoop.hbase.regionserver.HRegionServer; import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.util.JVMClusterUtil; import org.apache.hadoop.hbase.util.Threads; import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.jdbc.FailoverPhoenixConnection; -import org.apache.phoenix.jdbc.HABaseIT; -import org.apache.phoenix.jdbc.HighAvailabilityGroup; -import org.apache.phoenix.jdbc.HighAvailabilityPolicy; -import org.apache.phoenix.jdbc.HighAvailabilityTestingUtility; -import org.apache.phoenix.jdbc.PhoenixDriver; +import org.apache.phoenix.jdbc.PhoenixResultSet; import org.apache.phoenix.query.PhoenixTestBuilder; import org.apache.phoenix.query.QueryConstants; import org.apache.phoenix.replication.reader.ReplicationLogProcessor; -import org.apache.phoenix.replication.tool.LogFileAnalyzer; -import org.apache.phoenix.util.TestUtil; -import org.junit.After; -import org.junit.AfterClass; -import org.junit.Before; +import org.apache.phoenix.util.CDCUtil; +import org.apache.phoenix.util.QueryUtil; import org.junit.BeforeClass; import org.junit.Ignore; -import org.junit.Rule; import org.junit.Test; import org.junit.experimental.categories.Category; -import org.junit.rules.TestName; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; @Category(NeedsOwnMiniClusterTest.class) -public class ReplicationLogGroupIT extends HABaseIT { - private static final Logger LOG = LoggerFactory.getLogger(ReplicationLogGroupIT.class); - - @Rule - public TestName name = new TestName(); - - private Properties clientProps = new Properties(); - private String haGroupName; - private HighAvailabilityGroup haGroup; - private ReplicationLogGroup logGroup; +public class ReplicationLogGroupIT extends ReplicationLogGroupBaseIT { @BeforeClass public static void doSetup() throws Exception { - conf1.setInt(PHOENIX_REPLICATION_ROUND_DURATION_SECONDS_KEY, 20); - CLUSTERS.start(); - DriverManager.registerDriver(PhoenixDriver.INSTANCE); - } - - @AfterClass - public static void tearDownAfterClass() throws Exception { - DriverManager.deregisterDriver(PhoenixDriver.INSTANCE); - CLUSTERS.close(); - } - - @Before - public void beforeTest() throws Exception { - LOG.info("Starting test {}", name.getMethodName()); - haGroupName = name.getMethodName(); - clientProps = HighAvailabilityTestingUtility.getHATestProperties(); - clientProps.setProperty(PHOENIX_HA_GROUP_ATTR, haGroupName); - CLUSTERS.initClusterRole(haGroupName, HighAvailabilityPolicy.FAILOVER); - haGroup = getHighAvailibilityGroup(CLUSTERS.getJdbcHAUrl(), clientProps); - LOG.info("Initialized haGroup {} with URL {}", haGroup, CLUSTERS.getJdbcHAUrl()); - logGroup = getReplicationLogGroup(); - } - - @After - public void afterTest() throws Exception { - LOG.info("Starting cleanup for test {}", name.getMethodName()); - logGroup.close(); - LOG.info("Ending cleanup for test {}", name.getMethodName()); - } - - private ReplicationLogGroup getReplicationLogGroup() throws IOException { - HRegionServer rs = CLUSTERS.getHBaseCluster1().getHBaseCluster().getRegionServer(0); - return ReplicationLogGroup.get(conf1, rs.getServerName(), haGroupName); - } - - private Map> groupLogsByTable() throws Exception { - LogFileAnalyzer analyzer = new LogFileAnalyzer(); - // use peer cluster conf - analyzer.setConf(conf2); - Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); - LOG.info("Analyzing log files at {}", standByLogDir); - String[] args = { "--check", standByLogDir.toString() }; - assertEquals(0, analyzer.run(args)); - return analyzer.groupLogsByTable(standByLogDir.toString()); - } - - private int getCountForTable(Map> logsByTable, String tableName) - throws Exception { - List mutations = logsByTable.get(tableName); - return mutations != null ? mutations.size() : 0; - } - - private Map countRecordsByTable() throws Exception { - LogFileAnalyzer analyzer = new LogFileAnalyzer(); - // use peer cluster conf - analyzer.setConf(conf2); - Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); - return analyzer.countRecordsByTable(standByLogDir.toString()); - } - - private void verifyReplication(Map expected) throws Exception { - // first close the logGroup - logGroup.close(); - Map> mutationsByTable = groupLogsByTable(); - dumpTableLogCount(mutationsByTable); - for (Map.Entry entry : expected.entrySet()) { - String tableName = entry.getKey(); - int expectedMutationCount = entry.getValue(); - List mutations = mutationsByTable.get(tableName); - int actualMutationCount = mutations != null ? mutations.size() : 0; - try { - if (!tableName.equals(SYSTEM_CATALOG_NAME)) { - assertEquals(String.format("For table %s", tableName), expectedMutationCount, - actualMutationCount); - } else { - // special handling for syscat - assertTrue("For SYSCAT", actualMutationCount >= expectedMutationCount); - } - } catch (AssertionError e) { - // create a regular connection - try (Connection conn = DriverManager.getConnection(CLUSTERS.getJdbcUrl1(haGroup))) { - TestUtil.dumpTable(conn, TableName.valueOf(tableName)); - throw e; - } - } - } - } - - private void dumpTableLogCount(Map> mutationsByTable) { - LOG.info("Dump table log count for test {}", name.getMethodName()); - for (Map.Entry> table : mutationsByTable.entrySet()) { - LOG.info("#Log entries for {} = {}", table.getKey(), table.getValue().size()); - } - } - - private void moveRegionToServer(TableName tableName, ServerName sn) throws Exception { - HBaseTestingUtility util = CLUSTERS.getHBaseCluster1(); - try (RegionLocator locator = util.getConnection().getRegionLocator(tableName)) { - String regEN = locator.getAllRegionLocations().get(0).getRegionInfo().getEncodedName(); - while (!sn.equals(locator.getAllRegionLocations().get(0).getServerName())) { - LOG.info("Moving region {} of table {} to server {}", regEN, tableName, sn); - util.getAdmin().move(Bytes.toBytes(regEN), sn); - Thread.sleep(100); - } - LOG.info("Moved region {} of table {} to server {}", regEN, tableName, sn); - } - } - - private PhoenixTestBuilder.SchemaBuilder createViewHierarchy() throws Exception { - // Define the test schema. - // 1. Table with columns => (ORG_ID, KP, COL1, COL2, COL3), PK => (ORG_ID, KP) - // 2. GlobalView with columns => (ID, COL4, COL5, COL6), PK => (ID) - // 3. Tenant with columns => (ZID, COL7, COL8, COL9), PK => (ZID) - final PhoenixTestBuilder.SchemaBuilder schemaBuilder = - new PhoenixTestBuilder.SchemaBuilder(CLUSTERS.getJdbcHAUrl()); - PhoenixTestBuilder.SchemaBuilder.ConnectOptions connectOptions = - new PhoenixTestBuilder.SchemaBuilder.ConnectOptions(); - connectOptions.setConnectProps(clientProps); - PhoenixTestBuilder.SchemaBuilder.TableOptions tableOptions = - PhoenixTestBuilder.SchemaBuilder.TableOptions.withDefaults(); - PhoenixTestBuilder.SchemaBuilder.GlobalViewOptions globalViewOptions = - PhoenixTestBuilder.SchemaBuilder.GlobalViewOptions.withDefaults(); - PhoenixTestBuilder.SchemaBuilder.TenantViewOptions tenantViewWithOverrideOptions = - PhoenixTestBuilder.SchemaBuilder.TenantViewOptions.withDefaults(); - PhoenixTestBuilder.SchemaBuilder.TenantViewIndexOptions tenantViewIndexOverrideOptions = - PhoenixTestBuilder.SchemaBuilder.TenantViewIndexOptions.withDefaults(); - schemaBuilder.withConnectOptions(connectOptions).withTableOptions(tableOptions) - .withGlobalViewOptions(globalViewOptions).withTenantViewOptions(tenantViewWithOverrideOptions) - .withTenantViewIndexOptions(tenantViewIndexOverrideOptions).buildWithNewTenant(); - return schemaBuilder; - } - - private void replayAndVerifyAcrossClusters(List ddlStatements, String... tablesToVerify) - throws Exception { - Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); - - // Create the same schema on cluster 2 - try (Connection conn2 = CLUSTERS.getCluster2Connection(haGroup)) { - for (String ddl : ddlStatements) { - conn2.createStatement().execute(ddl); - } - conn2.commit(); - } - - // Replay replication log on cluster 2 - FileSystem fs = standByLogDir.getFileSystem(conf2); - List logFiles = findLogFiles(standByLogDir, fs); - assertTrue("Should have at least one log file", !logFiles.isEmpty()); - ReplicationLogProcessor processor = ReplicationLogProcessor.get(conf2, haGroupName); - try { - for (Path logFile : logFiles) { - LOG.info("Replaying log file: {}", logFile); - processor.processLogFile(fs, logFile); - } - } finally { - processor.close(); - } - - // Verify tables match across clusters at the HBase cell level - for (String table : tablesToVerify) { - assertTablesEqualAcrossClusters(table); - } + // Disable the IndexCDCConsumer on both clusters: CDC-index regeneration is verified directly, + // and the consumer's downstream secondary-index convergence is out of scope for these tests. + conf1.setBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, false); + conf2.setBoolean(PHOENIX_INDEX_CDC_CONSUMER_ENABLED, false); + setupClusters(); } @Test @@ -297,7 +116,7 @@ public void testUpsertSelectReplicatesViaCloneConnection() throws Exception { Map expected = Maps.newHashMap(); expected.put(sourceTable, rowCount); // direct upserts on the parent connection - expected.put(targetTable, rowCount); // upsert-select rows — currently fails: gets 0 + expected.put(targetTable, rowCount); // upsert-select rows expected.put(SYSTEM_CATALOG_NAME, 0); expected.put(SYSTEM_CHILD_LINK_NAME, 0); verifyReplication(expected); @@ -309,6 +128,7 @@ public void testAppendAndSync() throws Exception { final String indexName1 = "I_" + generateUniqueName(); final String indexName2 = "I_" + generateUniqueName(); final String indexName3 = "L_" + generateUniqueName(); + final String indexName4 = "U_" + generateUniqueName(); 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); @@ -318,6 +138,11 @@ public void testAppendAndSync() throws Exception { .format("create index if not exists %s on %s (val2) include (val1)", indexName2, tableName); String createLocalIndexDdl = String.format( "create local index if not exists %s on %s (id2,val1) include (val2)", indexName3, tableName); + // Uncovered index (no INCLUDE): rows are written UNVERIFIED in PRE and never marked VERIFIED in + // POST (IRO:2248), so the reader joins back to the data table. Both clusters write only + // UNVERIFIED rows, so cross-cluster cell equality still holds. + String createUncoveredIndexDdl = + String.format("create uncovered index if not exists %s on %s (val1)", indexName4, tableName); try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { @@ -325,6 +150,7 @@ public void testAppendAndSync() throws Exception { conn.createStatement().execute(createIndex1Ddl); conn.createStatement().execute(createIndex2Ddl); conn.createStatement().execute(createLocalIndexDdl); + conn.createStatement().execute(createUncoveredIndexDdl); conn.commit(); PreparedStatement stmt = conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?, ?)"); @@ -391,53 +217,214 @@ public void testAppendAndSync() throws Exception { } } - // Verify the system tables are never replicated, and flush the log group (verifyReplication - // closes it) before replay. We deliberately do NOT assert exact data/index mutation totals + // Assert the headline invariant of this feature: the index physical tables receive ZERO + // replication records (the standby regenerates index entries from the data record). System + // tables likewise never replicate. We deliberately do NOT assert exact data mutation totals // here: the multi-pass update workload's per-table counts are dominated by index-maintenance // internals (local-index key churn, verified/unverified empty-column writes) rather than by // the coalescing under test, and coalescing is mutation-count invariant by construction. The // authoritative correctness check for this workload is the cross-cluster cell-level equality // below; the record-count contract of coalescing is pinned separately in - // testAppendAndSyncSingleBatchRecordCount. + // testSingleBatchRecordCount. Map expected = Maps.newHashMap(); expected.put(SYSTEM_CATALOG_NAME, 0); expected.put(SYSTEM_CHILD_LINK_NAME, 0); + expected.put(indexName1, 0); + expected.put(indexName2, 0); + expected.put(indexName4, 0); verifyReplication(expected); // Replay on cluster 2 and verify cross-cluster cell-level equality - replayAndVerifyAcrossClusters( - Arrays.asList(createTableDdl, createIndex1Ddl, createIndex2Ddl, createLocalIndexDdl), - tableName, indexName1, indexName2); + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndex1Ddl, createIndex2Ddl, + createLocalIndexDdl, createUncoveredIndexDdl), tableName, indexName1, indexName2, + indexName4); } } /** - * Pins the per-batch coalescing contract: one server-side batch on a table with one index emits - * exactly three log records -- one for the data table, one for the index PRE phase, and one for - * the index POST phase -- regardless of how many rows the batch contains. Before coalescing this - * batch would have produced one record per mutation (3 rows x ~3 mutations each); coalescing - * collapses each (table, phase) into a single cell-stream record. Cross-cluster cell equality - * confirms the collapsed records still reconstruct the correct mutations on the standby. + * Uncovered-index-only variant exercising the path where the active NEVER calls + * {@code getCurrentRowStates}. The skip at {@code IndexRegionObserver:2513-2519} fires when the + * only index is uncovered and every mutation already carries the indexed column + * ({@code isPartialUncoveredIndexMutation == false}). With nothing read into + * {@code dataRowStates}, the active ships NO pre-image cell; the standby receives a + * self-contained full mutation, takes the same skip, and regenerates the uncovered index purely + * from the data cells. {@link #testAppendAndSync}'s uncovered index cannot reach this path: that + * table has a global index (forcing the read) and does val2-only updates (partial w.r.t. the val1 + * index). */ @Test - public void testAppendAndSyncSingleBatchRecordCount() throws Exception { + public void testUncoveredIndexNoCurrentRowState() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "U_" + generateUniqueName(); + String createTableDdl = String.format( + "create table if not exists %s (id integer not null primary key, val1 varchar, val2 varchar)", + tableName); + String createIndexDdl = + String.format("create uncovered index if not exists %s on %s (val1)", indexName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createIndexDdl); + conn.commit(); + + // Full upserts (all columns present), so the indexed column val1 is always supplied and the + // batch stays non-partial, keeping the active on the skip path. + PreparedStatement stmt = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?)"); + int rowCount = 10; + for (int i = 0; i < rowCount; ++i) { + stmt.setInt(1, i); + stmt.setString(2, "val1_" + i); + stmt.setString(3, "val2_" + i); + stmt.executeUpdate(); + } + conn.commit(); + + // Re-upsert the same rows as full mutations (val1 unchanged, val2 changed) so the index key + // is stable and the batch remains non-partial. + for (int i = 0; i < rowCount; ++i) { + stmt.setInt(1, i); + stmt.setString(2, "val1_" + i); + stmt.setString(3, "val2_updated_" + i); + stmt.executeUpdate(); + } + conn.commit(); + + // The uncovered index physical table must receive zero replication records. + Map expected = Maps.newHashMap(); + expected.put(SYSTEM_CATALOG_NAME, 0); + expected.put(SYSTEM_CHILD_LINK_NAME, 0); + expected.put(indexName, 0); + verifyReplication(expected); + + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndexDdl), tableName, + indexName); + } + } + + /** + * Local-index-only variant of {@link #testAppendAndSync}: the table has a local index but no + * global/uncovered/transform index. Such a table never enters the global-index branch that + * captures and ships the per-row PRE_IMAGE, so the active must capture it from the local-index + * prior-row scan instead (see {@code IndexRegionObserver.captureLocalIndexPreImageCells}); + * without that the standby's {@code PreImageLocalTable} would have no prior state and would miss + * covered columns and old-key tombstones. {@code testAppendAndSync} has both index types and so + * would not exercise this path. Local-index cells live in the data table's own {@code L#0} + * family, so verifying the data table cross-cluster also verifies the regenerated local index. + */ + @Test + public void testLocalIndexOnly() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String localIndexName = "L_" + generateUniqueName(); + 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); + String createLocalIndexDdl = + String.format("create local index if not exists %s on %s (id2,val1) include (val2)", + localIndexName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createLocalIndexDdl); + conn.commit(); + + // Insert 50 rows (val2 null) across 5 commits. + PreparedStatement stmt = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?, ?)"); + for (int i = 0; i < 5; ++i) { + for (int j = 0; j < 10; ++j) { + stmt.setInt(1, i); + stmt.setInt(2, j); + stmt.setString(3, "abcdefghijklmnopqrstuvwxyz"); + stmt.setString(4, null); + stmt.executeUpdate(); + } + conn.commit(); + } + + // Update the covered column val2 only (local index row key unchanged): exercises carrying a + // covered cell forward from the pre-image. + PreparedStatement updateVal2 = + conn.prepareStatement("upsert into " + tableName + " (id1, id2, val2) VALUES(?, ?, ?)"); + for (int i = 0; i < 5; ++i) { + for (int j = 0; j < 10; ++j) { + updateVal2.setInt(1, i); + updateVal2.setInt(2, j); + updateVal2.setString(3, "val2_" + i + "_" + j); + updateVal2.executeUpdate(); + } + } + conn.commit(); + + // Update the indexed column val1 (local index row key CHANGES): exercises the old-key + // DeleteFamily tombstone, which requires the prior row state from the pre-image. + PreparedStatement updateVal1 = + conn.prepareStatement("upsert into " + tableName + " (id1, id2, val1) VALUES(?, ?, ?)"); + for (int i = 0; i < 5; ++i) { + for (int j = 0; j < 10; ++j) { + updateVal1.setInt(1, i); + updateVal1.setInt(2, j); + updateVal1.setString(3, "newval1_" + i + "_" + j); + updateVal1.executeUpdate(); + } + } + conn.commit(); + + // Delete some rows: exercises full-row DeleteFamily on both data and local index. + PreparedStatement deleteStmt = + conn.prepareStatement("delete from " + tableName + " where id1 = ? and id2 = ?"); + for (int j = 0; j < 10; ++j) { + deleteStmt.setInt(1, 0); + deleteStmt.setInt(2, j); + deleteStmt.executeUpdate(); + } + conn.commit(); + + // Replay on cluster 2 and verify cross-cluster cell-level equality. The data table scan + // covers the L#0 local-index family, so this verifies the regenerated local index too. A + // local index shares the data region, so there is no separate index physical table to assert + // zero replication records on -- cross-cluster equality is the whole check. + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createLocalIndexDdl), tableName); + } + } + + /** + * Pins the per-batch coalescing contract: one server-side batch on a table with one global index + * and one local index emits exactly one log record -- the coalesced data-table cell stream + * carrying the per-row pre-image -- regardless of how many rows the batch contains. Neither index + * is replicated; the standby regenerates both from the data record plus its pre-image, so the + * global index table has no replication records of its own and the captured data-table cell + * stream carries no local-index ({@code L#}) cells. This is the explicit must-have check for the + * global + local scenario: local-index updates run after pre-image capture, so they are never in + * the shipped stream and the standby cannot double-write them. Cross-cluster cell equality then + * confirms the single collapsed record still reconstructs the correct data, global index, and (in + * the data table's own {@code L#0} family) local index on the standby. + */ + @Test + public void testSingleBatchRecordCount() throws Exception { final String tableName = "T_" + generateUniqueName(); final String indexName = "I_" + generateUniqueName(); + final String localIndexName = "L_" + generateUniqueName(); String createTableDdl = String.format( "create table if not exists %s (id integer not null primary key, val1 varchar, val2 varchar)", tableName); String createIndexDdl = String .format("create index if not exists %s on %s (val1) include (val2)", indexName, tableName); + String createLocalIndexDdl = String.format( + "create local index if not exists %s on %s (val2) include (val1)", localIndexName, tableName); try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { conn.createStatement().execute(createTableDdl); conn.createStatement().execute(createIndexDdl); + conn.createStatement().execute(createLocalIndexDdl); conn.commit(); // Insert several rows and commit them as a SINGLE batch (autocommit off, one commit()). All - // rows in this batch coalesce into one data-table record plus one PRE and one POST record on - // the index table. + // rows in this batch coalesce into one data-table record; index entries are regenerated on + // the standby, so the index table contributes no replication records. PreparedStatement stmt = conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?)"); int rowCount = 5; @@ -455,12 +442,26 @@ public void testAppendAndSyncSingleBatchRecordCount() throws Exception { LOG.info("Records by table: {}", recordsByTable); assertEquals("Data table should have exactly one coalesced record for the batch", Integer.valueOf(1), recordsByTable.get(tableName)); - assertEquals("Index table should have exactly two records (PRE + POST) for the batch", - Integer.valueOf(2), recordsByTable.get(indexName)); + assertNull("Global index table should have no replication records; entries are regenerated" + + " standby", recordsByTable.get(indexName)); - // Replay on cluster 2 and verify cross-cluster cell-level equality. - replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndexDdl), tableName, - indexName); + // The captured data-table cell stream must carry no local-index (L#) cells: local-index + // updates run after pre-image capture and so are never shipped. The standby regenerates them. + Map> logsByTable = groupLogsByTable(); + for (Mutation m : logsByTable.get(tableName)) { + for (Cell cell : m.getFamilyCellMap().values().stream().flatMap(List::stream) + .collect(Collectors.toList())) { + String family = Bytes.toString(CellUtil.cloneFamily(cell)); + assertFalse( + "Captured data record must not contain local-index (L#) cells, found family " + family, + family.startsWith(QueryConstants.LOCAL_INDEX_COLUMN_FAMILY_PREFIX)); + } + } + + // Replay on cluster 2 and verify cross-cluster cell-level equality. Verifying the data table + // also verifies the regenerated local index, whose cells live in the data table's L#0 family. + replayAndVerifyAcrossClusters( + Arrays.asList(createTableDdl, createIndexDdl, createLocalIndexDdl), tableName, indexName); } } @@ -524,7 +525,7 @@ public void testAppendAndSyncNoIndex() throws Exception { * that flow through the coprocessor merge path the codec must round-trip correctly. */ @Test - public void testAppendAndSyncOnDuplicateKeyUpdate() throws Exception { + public void testOnDuplicateKeyUpdate() throws Exception { final String tableName = "T_" + generateUniqueName(); String createTableDdl = String.format("create table if not exists %s " + "(pk varchar primary key, counter1 bigint, counter2 varchar)", tableName); @@ -573,13 +574,102 @@ public void testAppendAndSyncOnDuplicateKeyUpdate() throws Exception { } } + /** + * Atomic + global index: ON DUPLICATE KEY UPDATE on a table that also has a global index covering + * the mutated column. The active resolves the on-dup before pre-image capture, so the + * post-resolution data cells (including any DeleteColumn cells the on-dup generates) are captured + * into the row's {@code (row, ts)} group with their pre-image; the global index is not + * replicated. On the standby the reconstructed mutations carry no {@code ATOMIC_OP_ATTRIB} (it is + * not a replication attribute), so {@code identifyMutationTypes} leaves {@code hasAtomic} false: + * the standby does not re-resolve the on-dup and the {@code Preconditions.checkState} guard + * against active-side resolution flags does not fire. Indexing the on-dup-mutated column makes + * its index key churn (Delete old key + Put new key) across rounds. Cross-cluster cell equality + * on the data and index tables confirms the standby regenerates the index consistently with the + * active. + *

+ * Also covers returnResult + global index: a single-row atomic upsert with {@code RETURNING *} + * sets {@code RETURN_RESULT} on the active mutation, driving {@code context.returnResult} true on + * the active. {@code RETURN_RESULT} is likewise not a replication attribute, so the standby + * leaves {@code returnResult} false and {@code checkState} does not fire; the resolved cells + * (including the index-key change) replicate and regenerate. + */ + @Test + public void testOnDuplicateKeyUpdateWithIndex() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + String createTableDdl = String.format("create table if not exists %s " + + "(pk varchar primary key, counter1 bigint, counter2 varchar)", tableName); + String createIndexDdl = String.format( + "create index if not exists %s on %s (counter2) include (counter1)", indexName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createIndexDdl); + conn.commit(); + + // Initial inserts for 5 distinct rows + PreparedStatement insert = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, 0, 'init')"); + for (int i = 0; i < 5; ++i) { + insert.setString(1, "row_" + i); + insert.executeUpdate(); + } + conn.commit(); + + // ON DUPLICATE KEY UPDATE — increment counter1 and rewrite the indexed counter2 a few times + // per row. Each round changes the index key, so the atomic path emits Delete(oldKey) + + // Put(newKey) index work that the standby must regenerate from the captured data cells. + String dml = "UPSERT INTO " + tableName + " VALUES(?, 0, ?) " + + "ON DUPLICATE KEY UPDATE counter1 = counter1 + 1, counter2 = ?"; + PreparedStatement update = conn.prepareStatement(dml); + conn.setAutoCommit(true); + for (int round = 0; round < 3; ++round) { + for (int i = 0; i < 5; ++i) { + update.setString(1, "row_" + i); + update.setString(2, "v" + round); + update.setString(3, "v" + round); + update.executeUpdate(); + } + } + + // Set the indexed column to null via ON DUPLICATE KEY UPDATE — generates DeleteColumn cells + // on the data row and deletes the index entry for the prior key. + String dmlNullify = + "UPSERT INTO " + tableName + " VALUES(?, 0, '') ON DUPLICATE KEY UPDATE counter2 = NULL"; + PreparedStatement nullify = conn.prepareStatement(dmlNullify); + for (int i = 0; i < 5; ++i) { + nullify.setString(1, "row_" + i); + nullify.executeUpdate(); + } + + // Single-row atomic upsert with RETURNING *. RETURN_RESULT is not a replication + // attribute, so the standby regenerates the index without re-resolving. + String dmlReturning = "UPSERT INTO " + tableName + " VALUES('row_0', 0, 'init') " + + "ON DUPLICATE KEY UPDATE counter1 = counter1 + 1, counter2 = 'returned' RETURNING *"; + if (isSetCorrectResultEnabledOnHBase()) { + Statement returning = conn.createStatement(); + ResultSet rs = returning.execute(dmlReturning) ? returning.getResultSet() : null; + assertNotNull("RETURNING * should produce a result set", rs); + assertTrue("RETURNING * should project the atomically updated row", rs.next()); + assertFalse("Single-row atomic upsert returns exactly one row", rs.next()); + } else { + conn.createStatement().executeUpdate(dmlReturning); + } + + // Replay on cluster 2 and verify cross-cluster cell-level equality on data and index. + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndexDdl), tableName, + indexName); + } + } + /** * Verifies cross-cluster cell-level equality after replay for a table with a Conditional TTL * expression. Conditional TTL adds coprocessor cells that get merged into the data mutation, * exercising the split-merged-mutation path. */ @Test - public void testAppendAndSyncConditionalTTL() throws Exception { + public void testConditionalTTL() throws Exception { final String tableName = "T_" + generateUniqueName(); String createTableDdl = String.format("create table if not exists %s (id1 integer not null, " + "id2 integer not null, val1 varchar, val2 varchar, expired boolean " @@ -630,6 +720,145 @@ public void testAppendAndSyncConditionalTTL() throws Exception { } } + /** + * Conditional TTL + global index: a table with a Conditional TTL expression and a global index + * covering the columns the TTL evaluation touches. The active evaluates the TTL before pre-image + * capture, so the pre-image reflects post-conditional-TTL state and the captured data cells + * (including the masking Deletes the TTL path generates) carry their {@code (row, ts)} group's + * pre-image; the global index is not replicated. On the standby the reconstructed mutations carry + * no {@code TTL} attribute (it is not a replication attribute), so {@code identifyMutationTypes} + * leaves {@code hasConditionalTTL} false: the standby does not re-evaluate the TTL and the + * {@code Preconditions.checkState} guard against active-side resolution flags does not fire. + * Cross-cluster cell equality on data and index confirms the standby regenerates the index + * consistently with the active's post-TTL state. + */ + @Test + public void testConditionalTTLWithIndex() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + String createTableDdl = String.format("create table if not exists %s (id1 integer not null, " + + "id2 integer not null, val1 varchar, val2 varchar, expired boolean " + + "constraint pk primary key (id1, id2)) TTL = 'expired = TRUE'", tableName); + // Conditional TTL requires every column the TTL expression references (here: expired) to be + // present in the index, so it is covered alongside the indexed val1 / included val2. + String createIndexDdl = String.format( + "create index if not exists %s on %s (val1) include (val2, expired)", indexName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createIndexDdl); + conn.commit(); + + PreparedStatement stmt = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?, ?, ?)"); + for (int i = 0; i < 5; ++i) { + for (int j = 0; j < 10; ++j) { + stmt.setInt(1, i); + stmt.setInt(2, j); + stmt.setString(3, "val1_" + i + "_" + j); + stmt.setString(4, j % 2 == 0 ? "val2_" + i + "_" + j : null); + stmt.setBoolean(5, false); + stmt.executeUpdate(); + } + conn.commit(); + } + + // Mark some rows expired + PreparedStatement expireStmt = conn + .prepareStatement("upsert into " + tableName + " (id1, id2, expired) VALUES(?, ?, true)"); + for (int j = 0; j < 5; ++j) { + expireStmt.setInt(1, 0); + expireStmt.setInt(2, j); + expireStmt.executeUpdate(); + } + conn.commit(); + + // Update the indexed column on expired rows — conditional TTL triggers extra CP cells on the + // update path and the index key churns (Delete old val1 + Put new val1). + PreparedStatement updateStmt = + conn.prepareStatement("upsert into " + tableName + " (id1, id2, val1) VALUES(?, ?, ?)"); + for (int j = 0; j < 5; ++j) { + updateStmt.setInt(1, 0); + updateStmt.setInt(2, j); + updateStmt.setString(3, "val11_" + 0 + "_" + j); + updateStmt.executeUpdate(); + } + conn.commit(); + + // Replay on cluster 2 and verify cross-cluster cell-level equality on data and index. + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createIndexDdl), tableName, + indexName); + } + } + + /** + * Plain CDC index (no downstream EVENTUAL secondary index). A CDC index is a STRONG-consistency + * uncovered index written inline on the data write path; its rowkey leads with + * {@code PARTITION_ID()} = the encoded data-table region name. The active ships only data cells + + * per-(row,ts) pre-image (no index records); the standby regenerates the CDC index rowkey with + * its OWN partition_id. With no EVENTUAL index, {@code IndexCDCConsumer} stays dormant on both + * clusters, so replay is deterministic. The data table is verified byte-equal; the CDC index is + * verified equal modulo its leading partition_id (which differs across clusters by design). + */ + @Test + public void testCDCIndex() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String cdcName = "CDC_" + generateUniqueName(); + final String cdcIndexName = CDCUtil.getCDCIndexName(cdcName); + String createTableDdl = String.format( + "create table if not exists %s (pk integer not null " + "primary key, a varchar, b varchar)", + tableName); + String createCdcDdl = String.format("create cdc if not exists %s on %s", cdcName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createCdcDdl); + conn.commit(); + + // Inserts across several commits so the active produces multiple batches. + PreparedStatement upsert = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?)"); + for (int i = 0; i < 10; ++i) { + upsert.setInt(1, i); + upsert.setString(2, "a_" + i); + upsert.setString(3, "b_" + i); + upsert.executeUpdate(); + conn.commit(); + } + + // Update a column on some rows (new CDC change events for those rows). + PreparedStatement update = + conn.prepareStatement("upsert into " + tableName + " (pk, a) VALUES(?, ?)"); + for (int i = 0; i < 5; ++i) { + update.setInt(1, i); + update.setString(2, "a2_" + i); + update.executeUpdate(); + } + conn.commit(); + + // Delete a few rows (CDC delete events). + PreparedStatement delete = + conn.prepareStatement("delete from " + tableName + " where pk = ?"); + for (int i = 5; i < 8; ++i) { + delete.setInt(1, i); + delete.executeUpdate(); + } + conn.commit(); + + // Replay on cluster 2 and verify the data table byte-equal; verify the CDC index modulo + // its leading partition_id. + replayAndVerifyAcrossClusters(Arrays.asList(createTableDdl, createCdcDdl), tableName); + assertCDCIndexEqualAcrossClusters(cdcIndexName); + // The cross-cluster compare above is symmetric, so it passes whether or not the serialized + // downstream-index payload is present. Assert it positively against the live config so a + // serialize=true run that failed to propagate would fail loudly rather than pass + // degenerately. + assertCDCIndexPayloadMatchesConfig(cdcIndexName); + } + } + /** * This test simulates RS crashes in the middle of write transactions after the edits have been * written to the WAL but before they have been replicated to the standby cluster. Those edits @@ -693,10 +922,9 @@ public void testWALRestore() throws Exception { try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { Map expected = Maps.newHashMap(); - // For each row 1 Put + 1 Delete (DeleteColumn) + // For each row 1 Put + 1 Delete (DeleteColumn). + // Index mutations are not replicated; the standby regenerates them. expected.put(tableName, rowCount * 2); - // unverified + verified + delete (Delete column) - expected.put(indexName, rowCount * 3); // 1 tenant view was created // expected.put(SYSTEM_CHILD_LINK_NAME, 1); // atleast 1 log entry for syscat @@ -721,65 +949,291 @@ public void testSystemTables() throws Exception { assertTrue(getCountForTable(systemTables, SYSTEM_CATALOG_NAME) > 0); } - private List findLogFiles(Path dir, FileSystem fs) throws IOException { - List files = new ArrayList<>(); - findLogFilesRecursive(dir, fs, files); - return files; - } + /** + * Verifies that when data mutations are replayed on the standby via ReplicationLogProcessor, + * IndexRegionObserver on the standby generates index mutations from the data mutations. The + * replication log contains only data table mutations (no index mutations). + */ + @Test + public void testIndexRegenerationOnStandby() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + int rowCount = 10; + + // Create table and index on cluster 1 and insert data + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement() + .execute(String.format("CREATE TABLE %s (ID1 INTEGER NOT NULL, ID2 INTEGER NOT NULL, " + + "VAL1 VARCHAR CONSTRAINT PK PRIMARY KEY (ID1, ID2))", tableName)); + conn.createStatement() + .execute(String.format("CREATE INDEX %s ON %s (VAL1)", indexName, tableName)); + conn.commit(); + PreparedStatement stmt = + conn.prepareStatement("UPSERT INTO " + tableName + " VALUES(?, ?, ?)"); + for (int i = 0; i < rowCount; i++) { + stmt.setInt(1, i); + stmt.setInt(2, i); + stmt.setString(3, "val_" + i); + stmt.executeUpdate(); + } + conn.commit(); + } + + // Get the standby log dir path before closing the logGroup + Path standByLogDir = logGroup.getOrCreatePeerShardManager().getRootDirectoryPath(); + + // Verify replication log has only data table mutations (no index mutations) + logGroup.close(); + Map> logsByTable = groupLogsByTable(); + dumpTableLogCount(logsByTable); + assertTrue("Replication log should contain data table mutations", + logsByTable.containsKey(tableName)); + assertFalse("Replication log should NOT contain index table mutations", + logsByTable.containsKey(indexName)); - private void findLogFilesRecursive(Path dir, FileSystem fs, List files) throws IOException { - if (!fs.exists(dir)) { - return; + // Debug: dump cell timestamps from first deserialized mutation + List dataMutations = logsByTable.get(tableName); + if (dataMutations != null && !dataMutations.isEmpty()) { + Mutation firstMut = dataMutations.get(0); + LOG.info("First mutation type={} ts={}", firstMut.getClass().getSimpleName(), + firstMut.getTimestamp()); + for (Map.Entry> entry : firstMut.getFamilyCellMap().entrySet()) { + for (Cell cell : entry.getValue()) { + LOG.info(" Cell: cf={} qual={} ts={} type={}", + Bytes.toStringBinary(CellUtil.cloneFamily(cell)), + Bytes.toStringBinary(CellUtil.cloneQualifier(cell)), cell.getTimestamp(), + cell.getType()); + } + } } - for (FileStatus status : fs.listStatus(dir)) { - if (status.isDirectory()) { - findLogFilesRecursive(status.getPath(), fs, files); - } else if (status.getPath().getName().endsWith(".plog")) { - files.add(status.getPath()); + + // Create the same table and index on cluster 2 + try (Connection conn2 = CLUSTERS.getCluster2Connection(haGroup)) { + conn2.createStatement().execute( + String.format("CREATE TABLE IF NOT EXISTS %s (ID1 INTEGER NOT NULL, ID2 INTEGER NOT NULL, " + + "VAL1 VARCHAR CONSTRAINT PK PRIMARY KEY (ID1, ID2))", tableName)); + conn2.createStatement() + .execute(String.format("CREATE INDEX IF NOT EXISTS %s ON %s (VAL1)", indexName, tableName)); + conn2.commit(); + } + + // Replay the replication log on cluster 2 + FileSystem fs = standByLogDir.getFileSystem(conf2); + List logFiles = findLogFiles(standByLogDir, fs); + LOG.info("Found {} log files to replay", logFiles.size()); + assertTrue("Should have at least one log file", logFiles.size() > 0); + + ReplicationLogProcessor processor = ReplicationLogProcessor.get(conf2, haGroupName); + try { + for (Path logFile : logFiles) { + LOG.info("Replaying log file: {}", logFile); + processor.processLogFile(fs, logFile); + } + } finally { + processor.close(); + } + + try (Connection conn1 = CLUSTERS.getCluster1Connection(haGroup); + Statement stmt = conn1.createStatement()) { + // Query the data table + try (ResultSet rs = stmt.executeQuery("SELECT /*+ NO_INDEX */ COUNT(*) FROM " + tableName)) { + assertTrue(rs.next()); + assertEquals("Data table on cluster 1 should have all rows", rowCount, rs.getInt(1)); + } + + try (ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM " + tableName)) { + PhoenixResultSet prs = rs.unwrap(PhoenixResultSet.class); + String explainPlan = QueryUtil.getExplainPlan(prs.getUnderlyingIterator()); + assertTrue(explainPlan.contains(indexName)); + assertTrue(rs.next()); + assertEquals("Index table on cluster 1 should have all rows", rowCount, rs.getInt(1)); } } + + // Verify the index table on cluster 2 has data (generated by IRO during replay) + try (Connection conn2 = CLUSTERS.getCluster2Connection(haGroup); + Statement stmt = conn2.createStatement()) { + // Query the data table + try (ResultSet rs = stmt.executeQuery("SELECT /*+ NO_INDEX */ COUNT(*) FROM " + tableName)) { + assertTrue(rs.next()); + assertEquals("Data table on cluster 2 should have all rows", rowCount, rs.getInt(1)); + } + + try (ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM " + tableName)) { + PhoenixResultSet prs = rs.unwrap(PhoenixResultSet.class); + String explainPlan = QueryUtil.getExplainPlan(prs.getUnderlyingIterator()); + assertTrue(explainPlan.contains(indexName)); + assertTrue(rs.next()); + assertEquals("Index table on cluster 2 should have all rows", rowCount, rs.getInt(1)); + } + } + + // Deep cell-level comparison of data and index tables across clusters + assertTablesEqualAcrossClusters(tableName); + assertTablesEqualAcrossClusters(indexName); } - private void assertTablesEqualAcrossClusters(String hbaseTableName) throws Exception { - TableName tn = TableName.valueOf(hbaseTableName); - try ( - org.apache.hadoop.hbase.client.Connection hconn1 = ConnectionFactory.createConnection(conf1); - org.apache.hadoop.hbase.client.Connection hconn2 = ConnectionFactory.createConnection(conf2); - Table table1 = hconn1.getTable(tn); Table table2 = hconn2.getTable(tn)) { - - Scan scan = new Scan(); - scan.readAllVersions(); - - try (ResultScanner scanner1 = table1.getScanner(scan); - ResultScanner scanner2 = table2.getScanner(scan)) { - int rowCount = 0; - while (true) { - Result r1 = scanner1.next(); - Result r2 = scanner2.next(); - if (r1 == null && r2 == null) { - break; - } - assertNotNull( - String.format("Table %s: cluster 2 has fewer rows at row %d", hbaseTableName, rowCount), - r2); - assertNotNull( - String.format("Table %s: cluster 1 has fewer rows at row %d", hbaseTableName, rowCount), - r1); - try { - Result.compareResults(r1, r2, true); - } catch (Exception e) { - LOG.error("Table {} row {} mismatch. Dumping both tables:", hbaseTableName, rowCount); - LOG.error("--- Cluster 1 ---"); - TestUtil.dumpTable(table1); - LOG.error("--- Cluster 2 ---"); - TestUtil.dumpTable(table2); - fail(String.format("Table %s row %d mismatch: %s", hbaseTableName, rowCount, - e.getMessage())); + /** + * Concurrent same-row writes on the active (modeled on + * {@code ConditionalTTLExpressionIT#testConcurrentUpserts}): many threads hammer a small row set + * with randomized null/value columns, so the active produces a large, interleaved stream of + * overlapping batches -- the same data row updated at many timestamps, often within a single + * coalesced standby mini-batch. Replaying that stream on the standby exercises the + * per-{@code (row, + * ts)} grouping under contention: each group must fold only its own pre-image and cells, with no + * leak across rows or timestamps. The active's outcome is nondeterministic, so the invariant is + * cross-cluster cell equality -- the standby must reproduce exactly whatever the active + * committed, for both the data table and the global index. + */ + @Test + public void testConcurrentUpserts() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + 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); + 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(); + } + + runConcurrentUpsertWorkload(tableName); + + // The global index physical table must receive zero replication records. + Map expected = Maps.newHashMap(); + expected.put(SYSTEM_CATALOG_NAME, 0); + expected.put(SYSTEM_CHILD_LINK_NAME, 0); + expected.put(indexName, 0); + verifyReplication(expected); + + // Replay the per-round log files concurrently to simulate multiple region servers draining + // shard files in parallel within the same round. + replayAndVerifyAcrossClusters(4, Arrays.asList(createTableDdl, createIndexDdl), tableName, + indexName); + } + + /** + * Local-index counterpart to {@link #testConcurrentUpserts}: the same concurrent same-row + * workload, but the table carries only a local index. This drives the {@code PreImageLocalTable} + * replay path under contention -- overlapping batches for one row spread across several round log + * files, then replayed in parallel. Each thread randomizes the indexed column {@code val1}, so a + * large fraction of updates move the local-index row key and must emit an old-key + * {@code DeleteFamily} tombstone built from that group's own pre-image; getting the + * per-{@code (row, ts)} grouping wrong would either leak a stale index row or drop a tombstone. + * The active's outcome is nondeterministic, so the invariant is cross-cluster cell equality: the + * standby must reproduce exactly what the active committed. Local-index cells live in the data + * table's own {@code L#0} family, so verifying the data table cross-cluster also verifies the + * regenerated local index. + */ + @Test + public void testConcurrentUpsertsLocalIndex() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String localIndexName = "L_" + generateUniqueName(); + 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); + String createLocalIndexDdl = String.format( + "create local index if not exists %s on %s (val1) include (val2)", localIndexName, tableName); + + try (FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps)) { + conn.createStatement().execute(createTableDdl); + conn.createStatement().execute(createLocalIndexDdl); + conn.commit(); + } + + runConcurrentUpsertWorkload(tableName); + + // Replay the per-round log files concurrently to simulate multiple region servers draining + // shard files in parallel within the same round. The data table scan covers the L#0 local-index + // family, so this verifies the regenerated local index too. A local index shares the data + // region, so there is no separate index physical table to assert zero replication records on -- + // cross-cluster equality is the whole check. + replayAndVerifyAcrossClusters(4, Arrays.asList(createTableDdl, createLocalIndexDdl), tableName); + } + + /** + * Drives the concurrent same-row upsert workload shared by {@link #testConcurrentUpserts} and + * {@link #testConcurrentUpsertsLocalIndex} against a table with columns {@code (id1, id2, val1, + * val2)}. Eight threads hammer a 20-row set with randomized null/value columns (the indexed + * column {@code val1} included, so index-key-moving updates and old-key tombstones both occur). + * The workload runs by wall clock rather than a fixed iteration count: a short burst would land + * in one 5s replication round, leaving a single populated log file and no concurrent same-row + * replay; committing continuously for several rounds spreads the same rows across several files, + * which is what makes the caller's parallel replay do real overlapping work. Fails the calling + * test if any thread errors or the workload does not finish within the timeout. + */ + private void runConcurrentUpsertWorkload(String tableName) throws InterruptedException { + final int nThreads = 8; + final int batchSize = 50; + final int nRows = 20; + final long deadline = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(25); + final CountDownLatch doneSignal = new CountDownLatch(nThreads); + final AtomicReference firstError = new AtomicReference<>(); + for (int t = 0; t < nThreads; t++) { + final int seed = t; + Thread thread = new Thread(() -> { + Random rand = new Random(seed); + try ( + FailoverPhoenixConnection conn = (FailoverPhoenixConnection) DriverManager + .getConnection(CLUSTERS.getJdbcHAUrl(), clientProps); + PreparedStatement ps = + conn.prepareStatement("upsert into " + tableName + " VALUES(?, ?, ?, ?)")) { + int i = 0; + while (System.currentTimeMillis() < deadline) { + ps.setInt(1, i % nRows); + ps.setInt(2, 0); + ps.setString(3, rand.nextBoolean() ? null : "v1_" + rand.nextInt(nRows)); + ps.setString(4, rand.nextBoolean() ? null : "v2_" + rand.nextInt()); + ps.executeUpdate(); + if ((i % batchSize) == 0) { + conn.commit(); + } + i++; } - rowCount++; + conn.commit(); + } catch (Throwable e) { + firstError.compareAndSet(null, e); + } finally { + doneSignal.countDown(); } - LOG.info("Table {} matches across clusters: {} rows verified", hbaseTableName, rowCount); - } + }); + thread.start(); } + assertTrue("Ran out of time waiting for concurrent upserts", + doneSignal.await(120, TimeUnit.SECONDS)); + if (firstError.get() != null) { + throw new AssertionError("A concurrent upsert thread failed", firstError.get()); + } + } + + private PhoenixTestBuilder.SchemaBuilder createViewHierarchy() throws Exception { + // Define the test schema. + // 1. Table with columns => (ORG_ID, KP, COL1, COL2, COL3), PK => (ORG_ID, KP) + // 2. GlobalView with columns => (ID, COL4, COL5, COL6), PK => (ID) + // 3. Tenant with columns => (ZID, COL7, COL8, COL9), PK => (ZID) + final PhoenixTestBuilder.SchemaBuilder schemaBuilder = + new PhoenixTestBuilder.SchemaBuilder(CLUSTERS.getJdbcHAUrl()); + PhoenixTestBuilder.SchemaBuilder.ConnectOptions connectOptions = + new PhoenixTestBuilder.SchemaBuilder.ConnectOptions(); + connectOptions.setConnectProps(clientProps); + PhoenixTestBuilder.SchemaBuilder.TableOptions tableOptions = + PhoenixTestBuilder.SchemaBuilder.TableOptions.withDefaults(); + PhoenixTestBuilder.SchemaBuilder.GlobalViewOptions globalViewOptions = + PhoenixTestBuilder.SchemaBuilder.GlobalViewOptions.withDefaults(); + PhoenixTestBuilder.SchemaBuilder.TenantViewOptions tenantViewWithOverrideOptions = + PhoenixTestBuilder.SchemaBuilder.TenantViewOptions.withDefaults(); + PhoenixTestBuilder.SchemaBuilder.TenantViewIndexOptions tenantViewIndexOverrideOptions = + PhoenixTestBuilder.SchemaBuilder.TenantViewIndexOptions.withDefaults(); + schemaBuilder.withConnectOptions(connectOptions).withTableOptions(tableOptions) + .withGlobalViewOptions(globalViewOptions).withTenantViewOptions(tenantViewWithOverrideOptions) + .withTenantViewIndexOptions(tenantViewIndexOverrideOptions).buildWithNewTenant(); + return schemaBuilder; } } 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 f3edb027f87..95b34ec6a3e 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 @@ -29,6 +29,8 @@ import java.io.IOException; import java.sql.Connection; import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.Statement; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -49,6 +51,7 @@ import org.apache.hadoop.hbase.CellBuilderFactory; import org.apache.hadoop.hbase.CellBuilderType; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.DoNotRetryIOException; import org.apache.hadoop.hbase.HRegionInfo; import org.apache.hadoop.hbase.NotServingRegionException; import org.apache.hadoop.hbase.TableName; @@ -67,8 +70,10 @@ import org.apache.hadoop.hbase.util.Pair; import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; import org.apache.phoenix.end2end.ParallelStatsDisabledIT; +import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.jdbc.PhoenixConnection; import org.apache.phoenix.query.QueryServices; +import org.apache.phoenix.replication.MutationCellGrouper; import org.apache.phoenix.replication.log.LogFile; import org.apache.phoenix.replication.log.LogFileReader; import org.apache.phoenix.replication.log.LogFileReaderContext; @@ -397,6 +402,71 @@ public void testCalculateRetryDelay() throws IOException { smallDelayProcessor.close(); } + /** + * A DoNotRetryIOException is deterministic, so processReplicationLogBatch must fail immediately + * instead of burning batchRetryCount+1 attempts. The exception is wrapped (as the batch client + * wraps it in a RetriesExhaustedException cause), so this also covers the cause-chain walk. + */ + @Test + public void testProcessBatchDoesNotRetryDoNotRetryIOException() throws Exception { + ReplicationLogProcessor spyProcessor = + Mockito.spy(new ReplicationLogProcessor(conf, testHAGroupName)); + try { + assertTrue("Test assumes a non-zero retry budget", spyProcessor.getBatchRetryCount() > 0); + IOException wrapped = + new IOException("outer", new DoNotRetryIOException("deterministic failure")); + Map> failed = + Collections.singletonMap(TableName.valueOf("T"), Collections.emptyList()); + Mockito.doReturn(new ReplicationLogProcessor.ApplyMutationBatchResult(failed, wrapped)) + .when(spyProcessor).applyMutations(Mockito.any()); + + try { + spyProcessor.processReplicationLogBatch( + Collections.singletonMap(TableName.valueOf("T"), Collections.emptyList())); + fail("Should propagate the non-retryable failure"); + } catch (IOException e) { + // Expected. + } + + // Exactly one attempt: no retries for a DoNotRetryIOException. + Mockito.verify(spyProcessor, Mockito.times(1)).applyMutations(Mockito.any()); + } finally { + spyProcessor.close(); + } + } + + /** + * A retryable failure still exhausts the full retry budget: batchRetryCount+1 attempts. + */ + @Test + public void testProcessBatchRetriesRetryableException() throws Exception { + ReplicationLogProcessor spyProcessor = + Mockito.spy(new ReplicationLogProcessor(conf, testHAGroupName)); + try { + IOException wrapped = + new IOException("outer", new NotServingRegionException("transient failure")); + Map> failed = + Collections.singletonMap(TableName.valueOf("T"), Collections.emptyList()); + Mockito.doReturn(new ReplicationLogProcessor.ApplyMutationBatchResult(failed, wrapped)) + .when(spyProcessor).applyMutations(Mockito.any()); + // Avoid real backoff sleeps in the retry loop. + Mockito.doReturn(0L).when(spyProcessor).calculateRetryDelay(Mockito.anyInt()); + + try { + spyProcessor.processReplicationLogBatch( + Collections.singletonMap(TableName.valueOf("T"), Collections.emptyList())); + fail("Should propagate the failure after exhausting retries"); + } catch (IOException e) { + // Expected. + } + + Mockito.verify(spyProcessor, Mockito.times(spyProcessor.getBatchRetryCount() + 1)) + .applyMutations(Mockito.any()); + } finally { + spyProcessor.close(); + } + } + /** * Tests that configuration parameters are properly read and applied. */ @@ -720,7 +790,13 @@ public void testProcessLogFileBatchSizeLogic() throws Exception { new Path(testFolder.newFile("testProcessLogFileBatchSizeLogic").toURI()); final String tableName = "T_" + generateUniqueName(); final int batchSize = 5; - final long batchSizeBytes = 1800; + // Sized so the first batch flushes on bytes (3 small + 1 big mutation) while the second flushes + // on count (5 small). Each reconstructed mutation carries the REPLICATED_MUTATION attribute + // (~120 bytes), so a stamped small mutation is ~464 bytes and the big one ~1024. First batch = + // 3*464+1024 = 2416 (must be >= threshold to flush on bytes); second batch = 5*464 = 2320 (must + // stay < threshold so it flushes on count, not bytes). The threshold must sit in (2320, 2416]; + // 2400 leaves headroom on both sides. + final long batchSizeBytes = 2400; // Create log file with mutations of varying sizes LogFileWriter writer = initLogFileWriter(batchSizeFilePath); @@ -898,6 +974,89 @@ public void testProcessLogFileBatchSizeLogic() throws Exception { spyProcessor.close(); } + /** + * A single record's body is a flat cell stream that reconstructs into multiple mutations of mixed + * type (e.g. a batch that upserts some rows and, on one of those rows, also nulls a column -- a + * same-row Put plus DeleteColumn). The batch size check is deferred to the record boundary, so + * all mutations of one record must land in the same processReplicationLogBatch call even when the + * record's mutation count exceeds the configured batch size. This protects the (row, ts) grouping + * the standby IRO relies on: splitting a same-row Put and Delete across two batches would let + * each fold against the shared pre-image independently instead of together. + */ + @Test + public void testProcessLogFileDoesNotSplitRecordAcrossBatches() throws Exception { + final Path filePath = + new Path(testFolder.newFile("testDoesNotSplitRecordAcrossBatches").toURI()); + final String tableName = "T_" + generateUniqueName(); + // Batch size smaller than the number of mutations in the big record below, so the old in-loop + // flush would have split the record (the rowA Put/Delete straddle the count-2 boundary). + final int batchSize = 2; + + LogFileWriter writer = initLogFileWriter(filePath); + + // Record 0: one record whose flat cell stream mixes Puts and a Delete and reconstructs into 4 + // mutations in cell order: Put(rowZ), Put(rowA), Delete(rowA), Put(rowB). rowA carries a + // same-row + // Put+DeleteColumn group; the Put lands at mutation index 1 and the Delete at index 2, so a + // count-2 flush would separate them. + Put putZ = LogFileTestUtil.newPut("rowZ", 1L, 1); + Put putA = LogFileTestUtil.newPut("rowA", 1L, 2); + Delete deleteA = LogFileTestUtil.newDelete("rowA", 1L, 1); + Put putB = LogFileTestUtil.newPut("rowB", 1L, 1); + List bigRecordMutations = new ArrayList<>(); + Collections.addAll(bigRecordMutations, putZ, putA, deleteA, putB); + List bigRecordCells = new ArrayList<>(); + for (Mutation m : bigRecordMutations) { + bigRecordCells.addAll(LogFileTestUtil.cellsOf(m)); + } + writer.append(tableName, 0, bigRecordCells); + + // Record 1: a single-mutation record that would coalesce onto a following batch. + Put tailPut = LogFileTestUtil.newPut("tailRow", 2L, 1); + writer.append(tableName, 1, LogFileTestUtil.cellsOf(tailPut)); + + writer.close(); + + Configuration testConf = new Configuration(conf); + testConf.setInt(ReplicationLogProcessor.REPLICATION_STANDBY_LOG_REPLAY_BATCH_SIZE, batchSize); + + ReplicationLogProcessor spyProcessor = + Mockito.spy(new ReplicationLogProcessor(testConf, testHAGroupName)); + + List>> capturedArguments = new ArrayList<>(); + Mockito.doAnswer(invocation -> { + Map> originalMap = invocation.getArgument(0); + capturedArguments.add(new HashMap<>(originalMap)); + return null; + }).when(spyProcessor).processReplicationLogBatch(Mockito.any(Map.class)); + + spyProcessor.processLogFile(localFs, filePath); + + TableName expectedTableName = TableName.valueOf(tableName); + + // The big record (4 mutations) crosses the size threshold at its own boundary and flushes as a + // single batch; the tail record forms the second batch. The record is never split, so the + // rowA Put and Delete stay together. + assertEquals("Record boundary should produce exactly 2 batches", 2, capturedArguments.size()); + + List firstBatch = capturedArguments.get(0).get(expectedTableName); + assertNotNull("First batch mutations should not be null", firstBatch); + assertEquals("First batch must hold the whole big record (all 4 mutations together)", 4, + firstBatch.size()); + for (int i = 0; i < bigRecordMutations.size(); i++) { + LogFileTestUtil.assertMutationEquals("First batch mutation " + i + " mismatch", + bigRecordMutations.get(i), firstBatch.get(i)); + } + + List secondBatch = capturedArguments.get(1).get(expectedTableName); + assertNotNull("Second batch mutations should not be null", secondBatch); + assertEquals("Second batch should hold the tail record", 1, secondBatch.size()); + LogFileTestUtil.assertMutationEquals("Second batch mutation mismatch", tailPut, + secondBatch.get(0)); + + spyProcessor.close(); + } + /** * Tests batching logic when processing log files with mutations for multiple tables. */ @@ -1981,4 +2140,156 @@ private Cell cloneCellWithCustomTimestamp(Cell cell, long timestamp) { .setType(cell.getType()) .setValue(cell.getValueArray(), cell.getValueOffset(), cell.getValueLength()).build(); } + + /** + * Out-of-order replay (the original blocker for PHOENIX-7931): two batches touch the same row, + * and the later one is replayed first. The standby regenerates the global index from each batch's + * shipped pre-image rather than from a region scan, so the result is order-independent. Scenario: + *

    + *
  • M1 (ts=100): full row insert {ID=row1, COL_1=A, COL_2=X}, pre-image = empty row
    • + *
  • M2 (ts=200): partial update {ID=row1, COL_1=B} (COL_2 not included), pre-image = {COL_1=A, + * COL_2=X} (the row state the active observed before M2)
    • + *
+ * Replayed out of order (M2 then M1), naive regeneration would read COL_2=null for M2 (the row is + * absent) and produce an inconsistent index entry (B, null). Because M2 carries its own + * pre-image, the standby derives nextState=(B, X) and the index entry for COL_1=B correctly holds + * COL_2=X. + *

+ * The fixture reproduces what the active capture pipeline ships: data cells plus one METAFAMILY + * pre-image cell per row (via {@link IndexRegionObserver#buildPreImageCell}), with the + * replication attribute envelope (empty INDEX_UUID forcing server-side PTable resolution, + * schema/table names). + */ + @Test + public void testOutOfOrderReplayRegeneratesConsistentIndex() throws Exception { + final String tableName = "T_" + generateUniqueName(); + final String indexName = "I_" + generateUniqueName(); + final long ts1 = 100000L; + final long ts2 = 200000L; + + // Create table with index covering COL_1 and including COL_2 + try (Connection conn = getConnection()) { + conn.createStatement().execute(String.format( + "CREATE TABLE %s (ID VARCHAR PRIMARY KEY, COL_1 VARCHAR, COL_2 VARCHAR)", tableName)); + conn.createStatement().execute( + String.format("CREATE INDEX %s ON %s (COL_1) INCLUDE (COL_2)", indexName, tableName)); + conn.commit(); + } + + // Generate M1: full row insert at ts=100 + List m1Mutations; + try (Connection conn = getConnection()) { + PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class); + pconn.createStatement() + .execute(String.format("UPSERT INTO %s VALUES ('row1', 'A', 'X')", tableName)); + m1Mutations = extractMutationsWithTimestamp(pconn, ts1); + } + + // Generate M2: partial update at ts=200 (only COL_1, no COL_2) + List m2Mutations; + Put m2PreImage; + try (Connection conn = getConnection()) { + PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class); + pconn.createStatement() + .execute(String.format("UPSERT INTO %s (ID, COL_1) VALUES ('row1', 'B')", tableName)); + m2Mutations = extractMutationsWithTimestamp(pconn, ts2); + // M2's pre-image is the full row state M1 wrote: {COL_1=A, COL_2=X}. This is what the active + // observed at lock time and shipped alongside M2. + m2PreImage = new Put(Bytes.toBytes("row1")); + for (Mutation m : m1Mutations) { + for (Cell cell : LogFileTestUtil.cellsOf(m)) { + m2PreImage.add(cell); + } + } + } + + // Write M1 to file1 (pre-image = empty row), M2 to file2 (pre-image = M1's state) + Path file1 = new Path(testFolder.newFile("file1.plog").toURI()); + Path file2 = new Path(testFolder.newFile("file2.plog").toURI()); + appendReplicatedBatch(file1, tableName, m1Mutations, null); + appendReplicatedBatch(file2, tableName, m2Mutations, m2PreImage); + + // Replay out of order: file2 (M2, ts=200) first, then file1 (M1, ts=100) + ReplicationLogProcessor processor = ReplicationLogProcessor.get(conf, testHAGroupName); + try { + processor.processLogFile(localFs, file2); + processor.processLogFile(localFs, file1); + } finally { + processor.close(); + } + + // Verify data table: correct due to HBase cell versioning. + // Final state is COL_1=B (ts=200 wins) and COL_2=X (ts=100, only version). + try (Connection conn = getConnection(); Statement stmt = conn.createStatement()) { + ResultSet rs = stmt + .executeQuery(String.format("SELECT COL_1, COL_2 FROM %s WHERE ID = 'row1'", tableName)); + assertTrue("Should have a row", rs.next()); + assertEquals("Data table COL_1 should be B (ts=200 wins)", "B", rs.getString(1)); + assertEquals("Data table COL_2 should be X (from M1)", "X", rs.getString(2)); + rs.close(); + } + + // Verify index table via index hint: the regenerated entry for COL_1=B must carry COL_2=X, + // matching the data table, because M2's own pre-image supplied COL_2 even though M2 was + // replayed + // first (when the row was absent). + try (Connection conn = getConnection(); Statement stmt = conn.createStatement()) { + ResultSet rs = stmt + .executeQuery(String.format("SELECT /*+ INDEX(%s %s) */ COL_2 FROM %s WHERE COL_1 = 'B'", + tableName, indexName, tableName)); + assertTrue("Index should have an entry for COL_1=B", rs.next()); + assertEquals("Index COL_2 should be X (consistent with data table)", "X", rs.getString(1)); + rs.close(); + } + } + + /** + * Append one replicated batch to a fresh log file, reproducing the active capture pipeline's wire + * format: each record's cell stream is the mutation's data cells plus one METAFAMILY pre-image + * cell, and the record carries the replication attribute envelope (empty INDEX_UUID + + * schema/table names). A {@code null} {@code preImage} encodes the empty-row sentinel. + */ + private void appendReplicatedBatch(Path file, String tableName, List mutations, + Put preImage) throws IOException { + List cells = MutationCellGrouper.buildReplicatedCells(mutations, preImage); + Map attrs = MutationCellGrouper.buildReplicationAttributes("", tableName); + LogFileWriter writer = initLogFileWriter(file); + try { + writer.append(tableName, -1, cells, attrs); + writer.sync(); + } finally { + writer.close(); + } + } + + private List extractMutationsWithTimestamp(PhoenixConnection pconn, long timestamp) + throws Exception { + List result = new ArrayList<>(); + Iterator>> iterator = pconn.getMutationState().toMutations(); + while (iterator.hasNext()) { + Pair> pair = iterator.next(); + for (Mutation mutation : pair.getSecond()) { + if (mutation instanceof Put) { + Put put = (Put) mutation; + Put newPut = new Put(put.getRow()); + newPut.setTimestamp(timestamp); + for (Cell cell : put.getFamilyCellMap().values().stream().flatMap(List::stream) + .collect(Collectors.toList())) { + newPut.add(cloneCellWithCustomTimestamp(cell, timestamp)); + } + result.add(newPut); + } else if (mutation instanceof Delete) { + Delete delete = (Delete) mutation; + Delete newDelete = new Delete(delete.getRow()); + newDelete.setTimestamp(timestamp); + for (Cell cell : delete.getFamilyCellMap().values().stream().flatMap(List::stream) + .collect(Collectors.toList())) { + newDelete.add(cloneCellWithCustomTimestamp(cell, timestamp)); + } + result.add(newDelete); + } + } + } + return result; + } } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java b/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java new file mode 100644 index 00000000000..3ae42cb36da --- /dev/null +++ b/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java @@ -0,0 +1,301 @@ +/* + * 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.hbase.index; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.DoNotRetryIOException; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.client.Delete; +import org.apache.hadoop.hbase.client.Mutation; +import org.apache.hadoop.hbase.client.Put; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.Test; + +/** + * Unit tests for the standby-side index-regeneration helpers in {@link IndexRegionObserver}: + * {@link IndexRegionObserver#decodePreImage}, {@link IndexRegionObserver#applyDeleteToPut}, + * {@link IndexRegionObserver#deriveNextState}, and + * {@link IndexRegionObserver#buildReplicatedRowGroups}. These exercise the (row, ts) fold that + * recovers the active-side {@code nextDataRowState} from the per-row PRE_IMAGE plus the replicated + * cell stream. The reader/reconstruct side (peeling the PRE_IMAGE cell off the wire and stamping + * the attribute) is covered separately by {@code MutationCellGrouperTest}. + */ +public class IndexRegionObserverReplayTest { + + private static final byte[] ROW = Bytes.toBytes("r1"); + private static final byte[] R2 = Bytes.toBytes("r2"); + private static final byte[] CF = Bytes.toBytes("0"); + private static final byte[] Q1 = Bytes.toBytes("c1"); + private static final byte[] Q2 = Bytes.toBytes("c2"); + private static final byte[] Q3 = Bytes.toBytes("c3"); + private static final long TS = 100L; + + /** A Put on the given row at the given ts, one cell per qualifier. */ + private static Put putRowTs(byte[] row, long ts, byte[]... qualifiers) { + Put put = new Put(row); + for (byte[] q : qualifiers) { + put.addColumn(CF, q, ts, Bytes.toBytes("v")); + } + return put; + } + + /** Stamps the given pre-image onto a mutation as its PRE_IMAGE attribute and returns it. */ + private static M withPreImage(M m, Put preImage) throws IOException { + m.setAttribute(IndexRegionObserver.PRE_IMAGE, IndexRegionObserver.encodePreImage(preImage)); + return m; + } + + // ---- decodePreImage ---- + + @Test + public void testDecodePreImageMissingAttributeThrows() { + Put m = putRowTs(ROW, TS, Q1); + try { + IndexRegionObserver.decodePreImage(m); + fail("expected DoNotRetryIOException when PRE_IMAGE attribute is absent"); + } catch (DoNotRetryIOException expected) { + assertTrue(expected.getMessage().contains(IndexRegionObserver.PRE_IMAGE)); + } catch (IOException e) { + fail("expected DoNotRetryIOException, got " + e); + } + } + + @Test + public void testDecodePreImageEmptySentinelReturnsNull() throws IOException { + Put m = putRowTs(ROW, TS, Q1); + m.setAttribute(IndexRegionObserver.PRE_IMAGE, HConstants.EMPTY_BYTE_ARRAY); + assertNull("zero-length PRE_IMAGE is the 'active saw empty row' sentinel", + IndexRegionObserver.decodePreImage(m)); + } + + @Test + public void testDecodePreImageRoundTrip() throws IOException { + Put preImage = putRowTs(ROW, TS, Q1, Q2); + Put carrier = withPreImage(new Put(ROW), preImage); + + Put decoded = IndexRegionObserver.decodePreImage(carrier); + assertTrue(decoded.has(CF, Q1)); + assertTrue(decoded.has(CF, Q2)); + assertTrue(CellUtil.matchingRow(decoded.getFamilyCellMap().get(CF).get(0), ROW)); + } + + // ---- applyDeleteToPut ---- + + @Test + public void testApplyDeleteToNullPutIsNull() { + Delete d = new Delete(ROW); + d.addColumns(CF, Q1, TS); + assertNull(IndexRegionObserver.applyDeleteToPut(d, null)); + } + + @Test + public void testApplyDeleteColumnRemovesOnlyThatColumn() { + Put put = putRowTs(ROW, TS, Q1, Q2); + Delete d = new Delete(ROW); + d.addColumns(CF, Q1, TS); + + Put result = IndexRegionObserver.applyDeleteToPut(d, put); + assertFalse("Q1 should be removed", result.has(CF, Q1)); + assertTrue("Q2 should remain", result.has(CF, Q2)); + } + + @Test + public void testApplyDeleteFamilyRemovesWholeFamily() { + Put put = putRowTs(ROW, TS, Q1, Q2); + Delete d = new Delete(ROW); + d.addFamily(CF, TS); + + Put result = IndexRegionObserver.applyDeleteToPut(d, put); + assertNull("removing the only family empties the row", result); + } + + @Test + public void testApplyDeleteEmptiesRowReturnsNull() { + Put put = putRowTs(ROW, TS, Q1); + Delete d = new Delete(ROW); + d.addColumns(CF, Q1, TS); + + assertNull("deleting the last column empties the row", + IndexRegionObserver.applyDeleteToPut(d, put)); + } + + // ---- deriveNextState ---- + + @Test + public void testDeriveNextStateNoPreImageNoPutIsNull() throws IOException { + Delete d = new Delete(ROW); + d.addColumns(CF, Q1, TS); + assertNull("a Delete-only group with no pre-image yields no state", + IndexRegionObserver.deriveNextState(null, Collections. singletonList(d))); + } + + @Test + public void testDeriveNextStatePutOnNullPreImageInsert() throws IOException { + Put put = putRowTs(ROW, TS, Q1); + Put next = IndexRegionObserver.deriveNextState(null, Collections. singletonList(put)); + assertTrue(next.has(CF, Q1)); + } + + @Test + public void testDeriveNextStatePutMergesOntoPreImage() throws IOException { + Put preImage = putRowTs(ROW, TS, Q1); + Put put = putRowTs(ROW, TS, Q2); + Put next = + IndexRegionObserver.deriveNextState(preImage, Collections. singletonList(put)); + assertTrue("new column present", next.has(CF, Q2)); + assertTrue("pre-image column carried forward", next.has(CF, Q1)); + } + + @Test + public void testDeriveNextStatePutThenDelete() throws IOException { + Put preImage = putRowTs(ROW, TS, Q1); + Put put = putRowTs(ROW, TS, Q2); + Delete del = new Delete(ROW); + del.addColumns(CF, Q1, TS); + + List group = Arrays. asList(put, del); + Put next = IndexRegionObserver.deriveNextState(preImage, group); + assertFalse("Q1 deleted", next.has(CF, Q1)); + assertTrue("Q2 added", next.has(CF, Q2)); + } + + @Test + public void testDeriveNextStateDeleteEmptiesRow() throws IOException { + Put preImage = putRowTs(ROW, TS, Q1); + Delete del = new Delete(ROW); + del.addFamily(CF, TS); + + Put next = + IndexRegionObserver.deriveNextState(preImage, Collections. singletonList(del)); + assertNull("deleting the family empties the derived state", next); + } + + // ---- buildReplicatedRowGroups ---- + + @Test + public void testBuildReplicatedRowGroupsSplitsByTimestamp() throws IOException { + Put g1 = withPreImage(putRowTs(ROW, 100L, Q1), null); + Put g2 = withPreImage(putRowTs(ROW, 200L, Q2), putRowTs(ROW, 200L, Q1)); + + List groups = + IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(g1, g2)); + + assertEquals("two distinct (row, ts) groups", 2, groups.size()); + assertEquals(100L, groups.get(0).ts); + assertEquals(200L, groups.get(1).ts); + assertNull("first group's pre-image is the empty-row sentinel", groups.get(0).preImage); + assertTrue("second group's pre-image carries Q1", groups.get(1).preImage.has(CF, Q1)); + } + + @Test + public void testBuildReplicatedRowGroupsEachGroupKeepsItsOwnPreImage() throws IOException { + // Two batches on the same row: each ships its own authoritative pre-image. The second group's + // pre-image must NOT be derived from the first group — it is the active's shipped value. + Put g1 = withPreImage(putRowTs(ROW, 100L, Q1), null); + Put g2 = withPreImage(putRowTs(ROW, 200L, Q2), putRowTs(ROW, 200L, Q1)); + + List groups = + IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(g1, g2)); + + // group 1: no pre-image + a Put(Q1) -> next has Q1 + assertTrue(groups.get(0).nextState.has(CF, Q1)); + // group 2: pre-image(Q1) + Put(Q2) -> next has both + assertTrue(groups.get(1).nextState.has(CF, Q1)); + assertTrue(groups.get(1).nextState.has(CF, Q2)); + } + + @Test + public void testBuildReplicatedRowGroupsMergesSameRowTs() throws IOException { + Put a = withPreImage(putRowTs(ROW, 100L, Q1), null); + Put b = withPreImage(putRowTs(ROW, 100L, Q2), null); + + List groups = + IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(a, b)); + + assertEquals("same (row, ts) collapses into one group", 1, groups.size()); + assertEquals(2, groups.get(0).mutations.size()); + assertTrue(groups.get(0).nextState.has(CF, Q1)); + assertTrue(groups.get(0).nextState.has(CF, Q2)); + } + + /** + * The canonical (row, ts) grouping case: one mini-batch carrying four mutations across three + * groups -- (R1, ts1) holds a Put and a Delete, (R1, ts2) a Put on the same row at a later ts, + * (R2, ts1) a Put on a different row. Proves the groups are isolated: each folds only its own + * cells onto its own pre-image, and the (R1, ts1) Put+Delete fold does not leak into (R1, ts2) or + * (R2, ts1). + */ + @Test + public void testBuildReplicatedRowGroupsMultiRowMultiTsIsolation() throws IOException { + long ts1 = 100L, ts2 = 200L; + + // (R1, ts1): pre-image {Q1, Q3}; Put A adds Q2, Delete C removes Q3 -> next {Q1, Q2}. + Put r1t1Put = withPreImage(putRowTs(ROW, ts1, Q2), putRowTs(ROW, ts1, Q1, Q3)); + Delete r1t1Del = new Delete(ROW); + r1t1Del.addColumns(CF, Q3, ts1); + withPreImage(r1t1Del, putRowTs(ROW, ts1, Q1, Q3)); + // (R1, ts2): pre-image {Q1}; Put B adds Q3 -> next {Q1, Q3}. + Put r1t2Put = withPreImage(putRowTs(ROW, ts2, Q3), putRowTs(ROW, ts2, Q1)); + // (R2, ts1): no pre-image; Put X adds Q1 -> next {Q1}. + Put r2t1Put = withPreImage(putRowTs(R2, ts1, Q1), null); + + List groups = IndexRegionObserver + .buildReplicatedRowGroups(Arrays. asList(r1t1Put, r1t1Del, r1t2Put, r2t1Put)); + + // (a) exactly three groups, in first-seen order. + assertEquals("three (row, ts) groups", 3, groups.size()); + IndexRegionObserver.ReplicatedRowGroup g1 = groups.get(0); + IndexRegionObserver.ReplicatedRowGroup g2 = groups.get(1); + IndexRegionObserver.ReplicatedRowGroup g3 = groups.get(2); + + // (d) each group carries its own (row, ts). + assertTrue(Bytes.equals(ROW, g1.row.copyBytesIfNecessary())); + assertEquals(ts1, g1.ts); + assertTrue(Bytes.equals(ROW, g2.row.copyBytesIfNecessary())); + assertEquals(ts2, g2.ts); + assertTrue(Bytes.equals(R2, g3.row.copyBytesIfNecessary())); + assertEquals(ts1, g3.ts); + + // (b) (R1, ts1): both Put A and Delete C folded onto the ts1 pre-image. + assertEquals("R1/ts1 group holds the Put and the Delete", 2, g1.mutations.size()); + assertTrue("Q1 carried from pre-image", g1.nextState.has(CF, Q1)); + assertTrue("Q2 added by Put A", g1.nextState.has(CF, Q2)); + assertFalse("Q3 removed by Delete C", g1.nextState.has(CF, Q3)); + + // (c) (R1, ts2): only Put B on the ts2 pre-image -- no leak from the ts1 group. + assertTrue("Q1 carried from ts2 pre-image", g2.nextState.has(CF, Q1)); + assertTrue("Q3 added by Put B", g2.nextState.has(CF, Q3)); + assertFalse("Q2 must not leak in from the ts1 group", g2.nextState.has(CF, Q2)); + + // (R2, ts1): independent row, no pre-image -- only Put X's Q1. + assertNull("R2 group has the empty-row sentinel pre-image", g3.preImage); + assertTrue("Q1 inserted by Put X", g3.nextState.has(CF, Q1)); + assertFalse("no leak from R1 groups", g3.nextState.has(CF, Q2)); + assertFalse("no leak from R1 groups", g3.nextState.has(CF, Q3)); + } +} diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java index 684fdebcd7c..08e1c3e081d 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java @@ -17,194 +17,441 @@ */ package org.apache.phoenix.replication; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import java.sql.Connection; +import java.sql.DriverManager; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; import java.util.List; +import java.util.Map; import org.apache.hadoop.hbase.Cell; import org.apache.hadoop.hbase.CellBuilderFactory; import org.apache.hadoop.hbase.CellBuilderType; import org.apache.hadoop.hbase.CellUtil; +import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.hadoop.hbase.util.Pair; +import org.apache.hadoop.hbase.wal.WALEdit; +import org.apache.phoenix.execute.MutationState; +import org.apache.phoenix.hbase.index.IndexRegionObserver; +import org.apache.phoenix.index.PhoenixIndexCodec; +import org.apache.phoenix.jdbc.PhoenixConnection; +import org.apache.phoenix.query.BaseConnectionlessQueryTest; import org.junit.Test; /** - * Unit tests for {@link MutationCellGrouper#splitCellsIntoMutations}, the replay-side inverse of - * per-batch cell coalescing. This algorithm is the correctness lynchpin of PHOENIX-7931: a - * regression here would silently merge or split mutations on the standby with no exception, so the - * row+type boundary behavior is pinned here at the unit level rather than only through the - * heavyweight cross-cluster IT. + * Unit tests for {@link MutationCellGrouper}. Real Phoenix UPSERT/DELETE statements are used to + * obtain mutations with the same cell shapes the production write path produces, so the tests + * exercise the row+type boundary algorithm against representative inputs (Put, DeleteFamily, + * DeleteColumn) without manually constructing cells. */ -public class MutationCellGrouperTest { +public class MutationCellGrouperTest extends BaseConnectionlessQueryTest { - private static final byte[] FAMILY = Bytes.toBytes("cf"); - private static final byte[] QUALIFIER = Bytes.toBytes("q"); - private static final long TS = 100L; + private static List getMutations(PhoenixConnection pconn) throws Exception { + List all = new ArrayList<>(); + Iterator>> it = pconn.getMutationState().toMutations(); + while (it.hasNext()) { + all.addAll(it.next().getSecond()); + } + return all; + } - private static Cell putCell(String row, String value) { - return CellBuilderFactory.create(CellBuilderType.DEEP_COPY).setRow(Bytes.toBytes(row)) - .setFamily(FAMILY).setQualifier(QUALIFIER).setTimestamp(TS).setType(Cell.Type.Put) - .setValue(Bytes.toBytes(value)).build(); + private static List flatten(List mutations) { + List cells = new ArrayList<>(); + for (Mutation m : mutations) { + cells.addAll(MutationCellGrouper.flattenCells(m)); + } + return cells; } - private static Cell deleteColumnCell(String row) { - return CellBuilderFactory.create(CellBuilderType.DEEP_COPY).setRow(Bytes.toBytes(row)) - .setFamily(FAMILY).setQualifier(QUALIFIER).setTimestamp(TS).setType(Cell.Type.DeleteColumn) - .build(); + // ---------- splitCellsIntoMutations + flattenCells round-trip ---------- + + @Test + public void testUpsertSingleRowRoundTrip() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement() + .execute("CREATE TABLE t (k integer not null primary key, a varchar, b varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa', 'bb')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(1, source.size()); + assertTrue(source.get(0) instanceof Put); + + List regrouped = MutationCellGrouper.splitCellsIntoMutations(flatten(source)); + + assertEquals(1, regrouped.size()); + assertTrue(regrouped.get(0) instanceof Put); + assertArrayEquals(source.get(0).getRow(), regrouped.get(0).getRow()); + } } - private static Cell deleteFamilyCell(String row) { - return CellBuilderFactory.create(CellBuilderType.DEEP_COPY).setRow(Bytes.toBytes(row)) - .setFamily(FAMILY).setQualifier(QUALIFIER).setTimestamp(TS).setType(Cell.Type.DeleteFamily) - .build(); + @Test + public void testUpsertMultipleRowsRoundTrip() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + conn.createStatement().execute("UPSERT INTO t VALUES(2, 'bb')"); + conn.createStatement().execute("UPSERT INTO t VALUES(3, 'cc')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(3, source.size()); + + List regrouped = MutationCellGrouper.splitCellsIntoMutations(flatten(source)); + + assertEquals(3, regrouped.size()); + for (Mutation m : regrouped) { + assertTrue(m instanceof Put); + } + } } - private static String rowOf(Mutation m) { - return Bytes.toString(m.getRow()); + @Test + public void testDeleteRowRoundTrip() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("DELETE FROM t WHERE k = 7"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(1, source.size()); + assertTrue(source.get(0) instanceof Delete); + + List regrouped = MutationCellGrouper.splitCellsIntoMutations(flatten(source)); + + assertEquals(1, regrouped.size()); + assertTrue(regrouped.get(0) instanceof Delete); + assertArrayEquals(source.get(0).getRow(), regrouped.get(0).getRow()); + } } @Test - public void testEmptyInputYieldsEmptyList() throws Exception { - List result = + public void testMixedUpsertAndDeleteAcrossRows() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + conn.createStatement().execute("DELETE FROM t WHERE k = 2"); + conn.createStatement().execute("UPSERT INTO t VALUES(3, 'cc')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(3, source.size()); + + // Assert source shape against row key (Phoenix's MutationState may reorder mutations). + Map> sourceShape = new HashMap<>(); + for (Mutation m : source) { + sourceShape.put(Bytes.toStringBinary(m.getRow()), m.getClass()); + } + + List regrouped = MutationCellGrouper.splitCellsIntoMutations(flatten(source)); + + assertEquals(3, regrouped.size()); + Map> regroupedShape = new HashMap<>(); + for (Mutation m : regrouped) { + regroupedShape.put(Bytes.toStringBinary(m.getRow()), m.getClass()); + } + assertEquals(sourceShape, regroupedShape); + } + } + + /** + * UPSERT that sets a column to NULL produces both Put cells (for the non-null columns and the + * empty value cell) AND a DeleteColumn cell (for the explicit NULL). The grouper splits a mixed + * Put/DeleteColumn cell run for one row into one Put + one Delete, which is the correct + * primary-side shape that the standby applies to the same row in batch order. + */ + @Test + public void testUpsertWithNullProducesPutAndDeleteForSameRow() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute( + "CREATE TABLE t (k integer not null primary key, a varchar, b varchar, c varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa', NULL, 'cc')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + // Phoenix may produce one Put (with mixed cells) or a Put + a Delete pair; either way + // the row+type boundary algorithm must reproduce the same per-row cell sets after split. + Map> sourceByRow = cellsByRow(source); + List flattenedSource = flatten(source); + + List regrouped = MutationCellGrouper.splitCellsIntoMutations(flattenedSource); + + Map> regroupedByRow = cellsByRow(regrouped); + assertEquals("regrouped row key set must match source", sourceByRow.keySet(), + regroupedByRow.keySet()); + for (String rowKey : sourceByRow.keySet()) { + assertEquals("cell count for row " + rowKey + " must round-trip", + sourceByRow.get(rowKey).size(), regroupedByRow.get(rowKey).size()); + } + + // The point of this test: the mixed Put/DeleteColumn run for the row must split into a Put + // (non-null columns + empty value cell) AND a Delete (the explicit NULL) -- a single merged + // Put with the same total cell count would pass the per-row count check above but be wrong. + String rowKey = sourceByRow.keySet().iterator().next(); + List putsForRow = new ArrayList<>(); + List deletesForRow = new ArrayList<>(); + for (Mutation m : regrouped) { + if (!Bytes.toStringBinary(m.getRow()).equals(rowKey)) { + continue; + } + (m instanceof Delete ? deletesForRow : putsForRow).add(m); + } + assertEquals("row must split into exactly one Put", 1, putsForRow.size()); + assertEquals("row must split into exactly one Delete", 1, deletesForRow.size()); + // Every cell in each split mutation must carry the matching type (no Put cell leaked into the + // Delete, or vice versa), and the two together must account for all the row's cells. + int putCells = MutationCellGrouper.flattenCells(putsForRow.get(0)).size(); + int deleteCells = MutationCellGrouper.flattenCells(deletesForRow.get(0)).size(); + for (Cell c : MutationCellGrouper.flattenCells(putsForRow.get(0))) { + assertTrue("Put must hold only non-delete cells", !CellUtil.isDelete(c)); + } + for (Cell c : MutationCellGrouper.flattenCells(deletesForRow.get(0))) { + assertTrue("Delete must hold only delete cells", CellUtil.isDelete(c)); + } + assertEquals("Put + Delete cells must account for all the row's cells", + sourceByRow.get(rowKey).size(), putCells + deleteCells); + } + } + + private static Map> cellsByRow(List mutations) { + Map> byRow = new HashMap<>(); + for (Mutation m : mutations) { + String key = Bytes.toStringBinary(m.getRow()); + byRow.computeIfAbsent(key, k -> new ArrayList<>()) + .addAll(MutationCellGrouper.flattenCells(m)); + } + return byRow; + } + + @Test + public void testEmptyInputProducesEmptyOutput() throws Exception { + List regrouped = MutationCellGrouper.splitCellsIntoMutations(Collections. emptyList()); - assertTrue("Empty input should produce no mutations", result.isEmpty()); + assertTrue(regrouped.isEmpty()); + } + + // ---------- reconstructMutations: pre-image peeling + REPLICATED_MUTATION/PRE_IMAGE ---------- + + private static Cell preImageCell(byte[] row, byte[] value) { + return CellBuilderFactory.create(CellBuilderType.SHALLOW_COPY).setRow(row) + .setFamily(WALEdit.METAFAMILY).setQualifier(IndexRegionObserver.PRE_IMAGE_WAL_QUALIFIER) + .setTimestamp(HConstants.LATEST_TIMESTAMP).setType(Cell.Type.Put).setValue(value).build(); } @Test - public void testSinglePutCell() throws Exception { - Cell cell = putCell("row1", "v1"); - List result = - MutationCellGrouper.splitCellsIntoMutations(Collections.singletonList(cell)); - assertEquals(1, result.size()); - assertTrue("Expected a Put", result.get(0) instanceof Put); - assertEquals("row1", rowOf(result.get(0))); - assertEquals(1, result.get(0).size()); + public void testReconstructPeelsPreImageCellAndStampsAttribute() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(1, source.size()); + byte[] row = source.get(0).getRow(); + List cells = flatten(source); + // Synthesize a pre-image carrier (just opaque bytes here — production carries PB Put). + byte[] preImageBytes = Bytes.toBytes("OPAQUE_PB_PUT"); + cells.add(preImageCell(row, preImageBytes)); + + List regrouped = + MutationCellGrouper.reconstructMutations(cells, Collections. emptyMap()); + + assertEquals("pre-image cell must not produce its own mutation", 1, regrouped.size()); + assertTrue(regrouped.get(0) instanceof Put); + assertArrayEquals(row, regrouped.get(0).getRow()); + assertArrayEquals(preImageBytes, + regrouped.get(0).getAttribute(IndexRegionObserver.PRE_IMAGE)); + assertNotNull("REPLICATED_MUTATION marker must be stamped on every reconstructed mutation", + regrouped.get(0).getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + } } @Test - public void testSingleDeleteCell() throws Exception { - Cell cell = deleteColumnCell("row1"); - List result = - MutationCellGrouper.splitCellsIntoMutations(Collections.singletonList(cell)); - assertEquals(1, result.size()); - assertTrue("Expected a Delete", result.get(0) instanceof Delete); - assertEquals("row1", rowOf(result.get(0))); + public void testReconstructEmptyPreImageSentinel() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + byte[] row = source.get(0).getRow(); + List cells = flatten(source); + cells.add(preImageCell(row, HConstants.EMPTY_BYTE_ARRAY)); + + List regrouped = + MutationCellGrouper.reconstructMutations(cells, Collections. emptyMap()); + + assertEquals(1, regrouped.size()); + byte[] attr = regrouped.get(0).getAttribute(IndexRegionObserver.PRE_IMAGE); + assertNotNull(attr); + assertEquals("empty value sentinel preserved", 0, attr.length); + assertNotNull("REPLICATED_MUTATION marker must be stamped on every reconstructed mutation", + regrouped.get(0).getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + } } @Test - public void testContiguousCellsSameRowAndTypeFormOneMutation() throws Exception { - List cells = new ArrayList<>(); - cells.add(putCell("row1", "v1")); - cells.add(putCell("row1", "v2")); - cells.add(putCell("row1", "v3")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals("Same row + same type cells must coalesce into one Put", 1, result.size()); - assertTrue(result.get(0) instanceof Put); - assertEquals(3, result.get(0).size()); + public void testReconstructStampsReplicationAttributesOnEveryMutation() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + conn.createStatement().execute("UPSERT INTO t VALUES(2, 'bb')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(2, source.size()); + + Map attrs = new HashMap<>(); + attrs.put(PhoenixIndexCodec.INDEX_UUID, Bytes.toBytes("uuid-1")); + attrs.put(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes("S")); + attrs.put(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + Bytes.toBytes("T")); + + List regrouped = MutationCellGrouper.reconstructMutations(flatten(source), attrs); + + assertEquals(2, regrouped.size()); + for (Mutation m : regrouped) { + for (Map.Entry e : attrs.entrySet()) { + assertArrayEquals("attribute " + e.getKey() + " must be on every mutation", e.getValue(), + m.getAttribute(e.getKey())); + } + assertNotNull("REPLICATED_MUTATION marker must be on every reconstructed mutation", + m.getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + assertNull("PRE_IMAGE must not be set when no pre-image cell is present", + m.getAttribute(IndexRegionObserver.PRE_IMAGE)); + } + } } @Test - public void testRowChangeStartsNewMutation() throws Exception { - List cells = new ArrayList<>(); - cells.add(putCell("row1", "v1")); - cells.add(putCell("row2", "v2")); - cells.add(putCell("row3", "v3")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals("Each distinct row should yield its own Put", 3, result.size()); - assertEquals("row1", rowOf(result.get(0))); - assertEquals("row2", rowOf(result.get(1))); - assertEquals("row3", rowOf(result.get(2))); + public void testReconstructAttachesPreImageOnlyToMatchingRow() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + conn.createStatement().execute("UPSERT INTO t VALUES(2, 'bb')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + byte[] row1 = source.get(0).getRow(); + byte[] row2 = source.get(1).getRow(); + List cells = flatten(source); + // Only row1 has a pre-image. + byte[] row1PreImage = Bytes.toBytes("PRE-1"); + cells.add(preImageCell(row1, row1PreImage)); + + List regrouped = + MutationCellGrouper.reconstructMutations(cells, Collections. emptyMap()); + + assertEquals(2, regrouped.size()); + for (Mutation m : regrouped) { + assertNotNull("REPLICATED_MUTATION marker must be on every reconstructed mutation", + m.getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + if (Bytes.equals(m.getRow(), row1)) { + assertArrayEquals(row1PreImage, m.getAttribute(IndexRegionObserver.PRE_IMAGE)); + } else if (Bytes.equals(m.getRow(), row2)) { + assertNull(m.getAttribute(IndexRegionObserver.PRE_IMAGE)); + } + } + } } - /** - * The case that justifies the class: a single row whose Put cells precede its Delete cells (e.g. - * an index row that is rewritten then partially deleted within one server-side batch) must split - * on the put-vs-delete boundary into a separate Put and Delete, never merge into one mutation. - */ + // ---------- extractReplicationAttributes ---------- + @Test - public void testPutThenDeleteSameRowSplitsOnTypeBoundary() throws Exception { - List cells = new ArrayList<>(); - cells.add(putCell("row1", "v1")); - cells.add(deleteColumnCell("row1")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals("Put and Delete on the same row must be two mutations", 2, result.size()); - assertTrue("First should be the Put", result.get(0) instanceof Put); - assertTrue("Second should be the Delete", result.get(1) instanceof Delete); - assertEquals("row1", rowOf(result.get(0))); - assertEquals("row1", rowOf(result.get(1))); + public void testExtractReplicationAttributesFiltersToWellKnownKeys() throws Exception { + Put p = new Put(Bytes.toBytes("r")); + p.setAttribute(PhoenixIndexCodec.INDEX_UUID, Bytes.toBytes("uuid")); + p.setAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes("S")); + p.setAttribute("UNRELATED_ATTRIBUTE", Bytes.toBytes("ignore me")); + p.setAttribute(IndexRegionObserver.REPLICATED_MUTATION, Bytes.toBytes("must-not-leak")); + p.setAttribute(IndexRegionObserver.PRE_IMAGE, Bytes.toBytes("also-must-not-leak")); + + Map extracted = MutationCellGrouper.extractReplicationAttributes(p); + + assertArrayEquals( + "INDEX_UUID must be normalized to empty so the standby resolves index " + + "maintainers from its own PTable rather than the active's server-cache key", + HConstants.EMPTY_BYTE_ARRAY, extracted.get(PhoenixIndexCodec.INDEX_UUID)); + assertArrayEquals(Bytes.toBytes("S"), + extracted.get(MutationState.MutationMetadataType.SCHEMA_NAME.toString())); + assertNull("non-replication attribute must be filtered out", + extracted.get("UNRELATED_ATTRIBUTE")); + assertNull("REPLICATED_MUTATION is reader-synthesized, must not appear in record attributes", + extracted.get(IndexRegionObserver.REPLICATED_MUTATION)); + assertNull("PRE_IMAGE is reader-synthesized, must not appear in record attributes", + extracted.get(IndexRegionObserver.PRE_IMAGE)); } @Test - public void testPutDeletePutOnThreeRowsYieldsThreeMutations() throws Exception { - List cells = new ArrayList<>(); - cells.add(putCell("rowA", "v1")); - cells.add(deleteColumnCell("rowB")); - cells.add(putCell("rowC", "v3")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals(3, result.size()); - assertTrue(result.get(0) instanceof Put); - assertTrue(result.get(1) instanceof Delete); - assertTrue(result.get(2) instanceof Put); - assertEquals("rowA", rowOf(result.get(0))); - assertEquals("rowB", rowOf(result.get(1))); - assertEquals("rowC", rowOf(result.get(2))); + public void testExtractReplicationAttributesEmptyForBareMutation() throws Exception { + Put p = new Put(Bytes.toBytes("r")); + Map extracted = MutationCellGrouper.extractReplicationAttributes(p); + assertTrue(extracted.isEmpty()); } - /** - * Documents that the boundary is keyed on the exact cell type, not merely put-vs-delete: adjacent - * DeleteColumn and DeleteFamily cells on the same row split into two separate Delete mutations. - * This mirrors HBase's ReplicationSink and is relied on so a future change does not silently - * coalesce distinct delete subtypes. - */ + // ---------- contiguity stress (pre-image cell never breaks groupable runs) ---------- + @Test - public void testAdjacentDeleteSubtypesSameRowSplit() throws Exception { - List cells = new ArrayList<>(); - cells.add(deleteColumnCell("row1")); - cells.add(deleteFamilyCell("row1")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals("Distinct delete subtypes must split into separate Deletes", 2, result.size()); - assertTrue(result.get(0) instanceof Delete); - assertTrue(result.get(1) instanceof Delete); + public void testPreImageCellInterleavedDoesNotCreateExtraMutation() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("UPSERT INTO t VALUES(1, 'aa')"); + conn.createStatement().execute("UPSERT INTO t VALUES(2, 'bb')"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + byte[] row1 = source.get(0).getRow(); + byte[] row2 = source.get(1).getRow(); + // Build a stream that interleaves pre-image cells between row buckets: + // [row1.data..., row1.preImage, row2.data..., row2.preImage] + List cells = new ArrayList<>(); + cells.addAll(MutationCellGrouper.flattenCells(source.get(0))); + cells.add(preImageCell(row1, Bytes.toBytes("PRE-1"))); + cells.addAll(MutationCellGrouper.flattenCells(source.get(1))); + cells.add(preImageCell(row2, Bytes.toBytes("PRE-2"))); + + List regrouped = + MutationCellGrouper.reconstructMutations(cells, Collections. emptyMap()); + + assertEquals("pre-image cells must be peeled before grouping", 2, regrouped.size()); + for (Mutation m : regrouped) { + assertTrue(m instanceof Put); + assertNotNull(m.getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + assertNotNull(m.getAttribute(IndexRegionObserver.PRE_IMAGE)); + } + } } /** - * The grouper keys boundaries off the immediately preceding cell only, so a row that recurs - * non-contiguously produces a separate mutation per contiguous run. This encodes the documented - * "global row ordering is not required" contract: rowA appearing twice with rowB between yields - * two rowA mutations, not a merged one. + * Guard against a regression where a pre-image cell that follows a row's last cell of a different + * type might be misgrouped. The test: a Delete row's cells (Type.DeleteFamily) followed by a + * Type.Put pre-image cell. Without peeling, this would split into a Delete plus a stray Put. With + * peeling it must split into exactly one Delete carrying the pre-image attribute. */ @Test - public void testNonContiguousSameRowYieldsSeparateMutations() throws Exception { - List cells = new ArrayList<>(); - cells.add(putCell("rowA", "v1")); - cells.add(putCell("rowB", "v2")); - cells.add(putCell("rowA", "v3")); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals(3, result.size()); - assertEquals("rowA", rowOf(result.get(0))); - assertEquals("rowB", rowOf(result.get(1))); - assertEquals("rowA", rowOf(result.get(2))); - } + public void testPreImageCellAfterDeleteCellsIsPeeled() throws Exception { + try (Connection conn = DriverManager.getConnection(getUrl())) { + conn.setAutoCommit(false); + conn.createStatement().execute("CREATE TABLE t (k integer not null primary key, a varchar)"); + conn.createStatement().execute("DELETE FROM t WHERE k = 5"); + List source = getMutations(conn.unwrap(PhoenixConnection.class)); + assertEquals(1, source.size()); + assertTrue(source.get(0) instanceof Delete); + byte[] row = source.get(0).getRow(); - @Test - public void testCellsArePreservedInResultMutations() throws Exception { - Cell put1 = putCell("row1", "v1"); - Cell put2 = putCell("row1", "v2"); - List cells = new ArrayList<>(); - cells.add(put1); - cells.add(put2); - List result = MutationCellGrouper.splitCellsIntoMutations(cells); - assertEquals(1, result.size()); - List grouped = result.get(0).getFamilyCellMap().get(FAMILY); - assertEquals(2, grouped.size()); - assertTrue( - CellUtil.equals(put1, grouped.get(0)) && CellUtil.matchingValue(put1, grouped.get(0))); - assertTrue( - CellUtil.equals(put2, grouped.get(1)) && CellUtil.matchingValue(put2, grouped.get(1))); + List cells = new ArrayList<>(); + cells.addAll(MutationCellGrouper.flattenCells(source.get(0))); + cells.add(preImageCell(row, Bytes.toBytes("PRE-DEL"))); + + List regrouped = + MutationCellGrouper.reconstructMutations(cells, Collections. emptyMap()); + + assertEquals(1, regrouped.size()); + assertTrue(regrouped.get(0) instanceof Delete); + assertArrayEquals(Bytes.toBytes("PRE-DEL"), + regrouped.get(0).getAttribute(IndexRegionObserver.PRE_IMAGE)); + assertNotNull(regrouped.get(0).getAttribute(IndexRegionObserver.REPLICATED_MUTATION)); + } } } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/ReplicationLogGroupTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/ReplicationLogGroupTest.java index 25afca09645..d74eab43ba7 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/ReplicationLogGroupTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/ReplicationLogGroupTest.java @@ -41,6 +41,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; @@ -111,15 +112,15 @@ public void testAppendAndSync() throws Exception { // Happens-before ordering verification, using Mockito's inOrder. Verify that the appends // happen before sync, and sync happened after appends. inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId1), - eq(LogFileTestUtil.cellsOf(put1))); + eq(LogFileTestUtil.cellsOf(put1)), any()); inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId2), - eq(LogFileTestUtil.cellsOf(put2))); + eq(LogFileTestUtil.cellsOf(put2)), any()); inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId3), - eq(LogFileTestUtil.cellsOf(put3))); + eq(LogFileTestUtil.cellsOf(put3)), any()); inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId4), - eq(LogFileTestUtil.cellsOf(put4))); + eq(LogFileTestUtil.cellsOf(put4)), any()); inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId5), - eq(LogFileTestUtil.cellsOf(put5))); + eq(LogFileTestUtil.cellsOf(put5)), any()); inOrder.verify(writer, times(1)).sync(); } @@ -151,7 +152,7 @@ public void testSyncFailureAndRetry() throws Exception { // Verify the sequence: append, sync (fail), sync (succeed on retry with same writer) InOrder inOrder = Mockito.inOrder(writer); inOrder.verify(writer, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writer, times(2)).sync(); // First fails, second succeeds } @@ -176,7 +177,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable { sleep(50); // Simulate slow processing return invocation.callRealMethod(); } - }).when(innerWriter).append(anyString(), anyLong(), any(List.class)); + }).when(innerWriter).append(anyString(), anyLong(), any(List.class), any()); // Fill up the ring buffer by sending enough events. for (int i = 0; i < TEST_RINGBUFFER_SIZE; i++) { @@ -214,7 +215,7 @@ public Object answer(InvocationOnMock invocation) throws Throwable { // Verify the append eventually happens on the writer. verify(innerWriter, timeout(10000).times(1)).append(eq(tableName), eq(myCommitId), - any(List.class)); + any(List.class), any()); } /** @@ -233,7 +234,7 @@ public void testAppendFailureAndRetry() throws Exception { // Configure writerBeforeRoll to fail on the first append call doThrow(new IOException("Simulated append failure")).when(writerBeforeRoll).append(anyString(), - anyLong(), any(List.class)); + anyLong(), any(List.class), any()); // Append data logGroup.append(tableName, commitId, put); @@ -246,10 +247,10 @@ public void testAppendFailureAndRetry() throws Exception { // Verify the sequence: append (fail), rotate, append (succeed), sync InOrder inOrder = Mockito.inOrder(writerBeforeRoll, writerAfterRoll); inOrder.verify(writerBeforeRoll, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writerBeforeRoll, times(0)).sync(); // We failed append, did not try inOrder.verify(writerAfterRoll, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); // Retry + eq(LogFileTestUtil.cellsOf(put)), any()); // Retry inOrder.verify(writerAfterRoll, times(1)).sync(); } @@ -358,7 +359,7 @@ public void testConcurrentProducers() throws Exception { // Verify that all of appends were processed by the internal writer. for (int i = 0; i < APPENDS_PER_THREAD * 2; i++) { final long commitId = i; - verify(innerWriter, times(1)).append(eq(tableName), eq(commitId), any(List.class)); + verify(innerWriter, times(1)).append(eq(tableName), eq(commitId), any(List.class), any()); } } @@ -400,10 +401,10 @@ public void testTimeBasedRotation() throws Exception { // Verify the sequence of operations InOrder inOrder = Mockito.inOrder(writerBeforeRotation, writerAfterRotation); inOrder.verify(writerBeforeRotation, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writerBeforeRotation, times(1)).sync(); inOrder.verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId + 1), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writerAfterRotation, times(1)).sync(); } @@ -443,7 +444,7 @@ public void testSizeBasedRotation() throws Exception { // Verify the final append went to the new writer verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(writerAfterRotation, times(1)).sync(); } @@ -529,11 +530,11 @@ public void testRotationTask() throws Exception { // Verify first batch went to initial writer verify(writerBeforeRotation, times(1)).append(eq(tableName), eq(1L), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(writerBeforeRotation, times(1)).sync(); // Verify second batch went to new writer (swap happened before append) verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId + 1), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(writerAfterRotation, times(1)).sync(); // Verify the initial writer was closed asynchronously verify(writerBeforeRotation, timeout(5000).times(1)).close(); @@ -589,13 +590,13 @@ public void testFailedRotation() throws Exception { // Verify operations went to the writers in the correct order InOrder inOrder = Mockito.inOrder(initialWriter, writerAfterRotate); inOrder.verify(initialWriter).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(initialWriter).sync(); inOrder.verify(initialWriter).append(eq(tableName), eq(commitId + 1), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(initialWriter).sync(); inOrder.verify(writerAfterRotate).append(eq(tableName), eq(commitId + 2), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writerAfterRotate).sync(); } @@ -651,7 +652,7 @@ public void testEventProcessingException() throws Exception { // Configure writer to throw a RuntimeException on append doThrow(new RuntimeException("Simulated critical error")).when(innerWriter).append(anyString(), - anyLong(), any(List.class)); + anyLong(), any(List.class), any()); // Append publishes to the ring buffer. The event handler catches the RuntimeException via // catch(Throwable), poisons itself, and fails the sync future. The producer receives @@ -789,12 +790,12 @@ public void testRotationDuringBatch() throws Exception { // 5 appends went to old writer (processed before rotation task fired) for (int i = 0; i < 5; i++) { inOrder.verify(writerBeforeRotation, times(1)).append(eq(tableName), eq(commitId + i), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); } // Swap happens before sync action: 5 records replayed into new writer for (int i = 0; i < 5; i++) { inOrder.verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId + i), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); } // Sync goes to new writer inOrder.verify(writerAfterRotation, times(1)).sync(); @@ -1048,7 +1049,7 @@ public void testAppendAfterCloseOnError() throws Exception { // Configure writer to throw RuntimeException on append doThrow(new RuntimeException("Simulated critical error")).when(innerWriter).append(anyString(), - anyLong(), any(List.class)); + anyLong(), any(List.class), any()); // Append publishes to the ring buffer. The event handler catches the RuntimeException, // poisons itself, and fails the sync future. The producer calls abort(). @@ -1087,7 +1088,7 @@ public void testSyncAfterCloseOnError() throws Exception { // Configure writer to throw RuntimeException on append doThrow(new RuntimeException("Simulated critical error")).when(innerWriter).append(anyString(), - anyLong(), any(List.class)); + anyLong(), any(List.class), any()); // Append publishes to the ring buffer. The event handler catches the RuntimeException, // poisons itself, and fails the sync future. The producer calls abort(). @@ -1194,7 +1195,8 @@ public Object answer(InvocationOnMock invocation) throws Throwable { sleep(50); // Delay to allow multiple events to be posted return invocation.callRealMethod(); } - }).when(innerWriter).append(eq(tableName), eq(commitId1), eq(LogFileTestUtil.cellsOf(put1))); + }).when(innerWriter).append(eq(tableName), eq(commitId1), eq(LogFileTestUtil.cellsOf(put1)), + any()); // Post appends and three syncs in quick succession. The first append will be delayed long // enough for the three syncs to appear in a single Disruptor batch. Then they should all @@ -1210,11 +1212,11 @@ public Object answer(InvocationOnMock invocation) throws Throwable { // one sync. InOrder inOrder = Mockito.inOrder(innerWriter); inOrder.verify(innerWriter, times(1)).append(eq(tableName), eq(commitId1), - eq(LogFileTestUtil.cellsOf(put1))); + eq(LogFileTestUtil.cellsOf(put1)), any()); inOrder.verify(innerWriter, times(1)).append(eq(tableName), eq(commitId2), - eq(LogFileTestUtil.cellsOf(put2))); + eq(LogFileTestUtil.cellsOf(put2)), any()); inOrder.verify(innerWriter, times(1)).append(eq(tableName), eq(commitId3), - eq(LogFileTestUtil.cellsOf(put3))); + eq(LogFileTestUtil.cellsOf(put3)), any()); inOrder.verify(innerWriter, times(1)).sync(); // Only one sync should be called } @@ -1318,12 +1320,12 @@ public void testInFlightAppendsReplayAfterModeSwitch() throws Exception { // invocation/stub state is not thread-safe and the partially-applied stub can be matched // against an unrelated method on the consumer thread. doThrow(new IOException("Simulate append failure")).when(writer).append(tableName, commitId5, - LogFileTestUtil.cellsOf(put5)); + LogFileTestUtil.cellsOf(put5), Collections.emptyMap()); // Rotated writers must also fail on the 5th append so the retry doesn't rescue the loop. doAnswer(invocation -> { LogFileWriter w = (LogFileWriter) invocation.callRealMethod(); doThrow(new IOException("Simulate append failure")).when(w).append(tableName, commitId5, - LogFileTestUtil.cellsOf(put5)); + LogFileTestUtil.cellsOf(put5), Collections.emptyMap()); return w; }).when(activeLog).createNewWriter(); @@ -1341,15 +1343,15 @@ public void testInFlightAppendsReplayAfterModeSwitch() throws Exception { // verify that all the in-flight appends and syncs are replayed on the new store and forward // writer inOrder.verify(storeAndForwardWriter, times(1)).append(eq(tableName), eq(commitId1), - eq(LogFileTestUtil.cellsOf(put1))); + eq(LogFileTestUtil.cellsOf(put1)), any()); inOrder.verify(storeAndForwardWriter, times(1)).append(eq(tableName), eq(commitId2), - eq(LogFileTestUtil.cellsOf(put2))); + eq(LogFileTestUtil.cellsOf(put2)), any()); inOrder.verify(storeAndForwardWriter, times(1)).append(eq(tableName), eq(commitId3), - eq(LogFileTestUtil.cellsOf(put3))); + eq(LogFileTestUtil.cellsOf(put3)), any()); inOrder.verify(storeAndForwardWriter, times(1)).append(eq(tableName), eq(commitId4), - eq(LogFileTestUtil.cellsOf(put4))); + eq(LogFileTestUtil.cellsOf(put4)), any()); inOrder.verify(storeAndForwardWriter, times(1)).append(eq(tableName), eq(commitId5), - eq(LogFileTestUtil.cellsOf(put5))); + eq(LogFileTestUtil.cellsOf(put5)), any()); inOrder.verify(storeAndForwardWriter, times(1)).sync(); } @@ -1415,16 +1417,16 @@ public void testReplayOnMidBatchSwap() throws Exception { // 3 appends went to old writer for (int i = 0; i < 3; i++) { inOrder.verify(writerBeforeRotation, times(1)).append(eq(tableName), eq(commitId + i), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); } // 3 records replayed into new writer for (int i = 0; i < 3; i++) { inOrder.verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId + i), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); } // 4th append goes to new writer inOrder.verify(writerAfterRotation, times(1)).append(eq(tableName), eq(commitId + 3), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); inOrder.verify(writerAfterRotation, times(1)).sync(); // Old writer closed async @@ -1467,11 +1469,11 @@ public void testRetryPicksUpStagedWriter() throws Exception { // Old writer: received the append only verify(initialWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); // New writer: received replayed append + successful sync verify(newWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(newWriter, times(1)).sync(); } @@ -1504,7 +1506,7 @@ public void testIdleLeaseRecoveryDrainsStagedWriter() throws Exception { // Simulate HDFS lease recovery breaking the old writer's stream. Installed while idle — before // forceRotation() publishes the swap event — so the consumer cannot race this stub install. doThrow(new IOException("Simulated broken stream after lease recovery")).when(initialWriter) - .append(anyString(), anyLong(), any(List.class)); + .append(anyString(), anyLong(), any(List.class), any()); doThrow(new IOException("Simulated broken stream after lease recovery")).when(initialWriter) .sync(); @@ -1521,7 +1523,7 @@ public void testIdleLeaseRecoveryDrainsStagedWriter() throws Exception { // New writer received the new append + sync (no replay — currentBatch was empty) verify(newWriter, times(1)).append(eq(tableName), eq(commitId + 1), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(newWriter, times(1)).sync(); // Await the async close of the old writer. This happens-after every invocation the swap makes @@ -1530,7 +1532,7 @@ public void testIdleLeaseRecoveryDrainsStagedWriter() throws Exception { // Old writer: only the pre-idle append + sync, nothing after the break verify(initialWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(initialWriter, times(1)).sync(); } @@ -1557,7 +1559,7 @@ public void testReplayFailureRetries() throws Exception { throw new IOException("Simulated transient HDFS error during replay"); } return appendInvocation.callRealMethod(); - }).when(w).append(anyString(), anyLong(), any(List.class)); + }).when(w).append(anyString(), anyLong(), any(List.class), any()); return w; }).when(activeLog).createNewWriter(); @@ -1579,11 +1581,11 @@ public void testReplayFailureRetries() throws Exception { // Final writer (W3): replayed r1+r2 then appended r3 — each exactly once. verify(finalWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(finalWriter, times(1)).append(eq(tableName), eq(commitId + 1), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(finalWriter, times(1)).append(eq(tableName), eq(commitId + 2), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(finalWriter, times(1)).sync(); } @@ -1604,7 +1606,7 @@ public void testErrorRecoveryRequestsNewWriter() throws Exception { // Configure initial writer's append to always fail (simulating broken HDFS stream) doThrow(new IOException("Simulated broken stream")).when(initialWriter).append(anyString(), - anyLong(), any(List.class)); + anyLong(), any(List.class), any()); // Append — attempt 1 fails on initialWriter, rotation requested, attempt 2 drains the // rotated writer and succeeds @@ -1616,10 +1618,10 @@ public void testErrorRecoveryRequestsNewWriter() throws Exception { // Old writer received 1 failed attempt verify(initialWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); // New writer received the successful append verify(newWriter, times(1)).append(eq(tableName), eq(commitId), - eq(LogFileTestUtil.cellsOf(put))); + eq(LogFileTestUtil.cellsOf(put)), any()); verify(newWriter, times(1)).sync(); } @@ -2120,7 +2122,7 @@ public void testPublishSwapEventOnFullRingBufferIsNoop() throws Exception { doAnswer(invocation -> { holdConsumer.await(); return invocation.callRealMethod(); - }).when(innerWriter).append(anyString(), anyLong(), any(List.class)); + }).when(innerWriter).append(anyString(), anyLong(), any(List.class), any()); Thread filler = new Thread(() -> { try { @@ -2239,7 +2241,7 @@ public void testReplicationSyncPathSimulator() throws Exception { LogFileTestUtil.newPut("row" + commitId + "_" + j, commitId, cellsPerMutation); batchCells.addAll(LogFileTestUtil.cellsOf(put)); } - logGroup.append(tableName, commitId, batchCells); + logGroup.append(tableName, commitId, batchCells, Collections.emptyMap()); totalProducerAppends.incrementAndGet(); } else { for (int j = 0; j < appendsPerSync; j++) { @@ -2364,9 +2366,11 @@ public void testSyncMetricsEmitted() throws Exception { // getCurrentMetricValues() snapshots and resets the histogram bins, so call it exactly once. ReplicationLogMetricValues values = logGroup.getMetrics().getCurrentMetricValues(); - // Nanosecond-resolution metrics never truncate to zero, so assert strictly positive. - assertTrue("appendTime should be > 0, got " + values.getAppendTimeMax(), - values.getAppendTimeMax() > 0); + // appendTime brackets only the uncontended ring-buffer publish -- the shortest interval of all + // these metrics. Nanosecond units are not nanosecond resolution: on a fast machine every + // publish can floor to a 0ns delta, so assert presence (>= 0) rather than strict positivity. + assertTrue("appendTime should be >= 0, got " + values.getAppendTimeMax(), + values.getAppendTimeMax() >= 0); assertTrue("ringBufferTime should be > 0, got " + values.getRingBufferTimeMax(), values.getRingBufferTimeMax() > 0); assertTrue("pendingSyncWaitTime should be > 0, got " + values.getPendingSyncWaitTimeMax(), diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java index 98e70aad863..2febc5a4267 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java @@ -38,8 +38,13 @@ import org.apache.hadoop.hbase.HBaseConfiguration; import org.apache.hadoop.hbase.HConstants; import org.apache.hadoop.hbase.client.Delete; +import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.phoenix.execute.MutationState; +import org.apache.phoenix.hbase.index.IndexRegionObserver; +import org.apache.phoenix.index.PhoenixIndexCodec; +import org.apache.phoenix.replication.ReplicationLogGroup; import org.junit.Assume; import org.junit.Test; import org.slf4j.Logger; @@ -742,4 +747,98 @@ private static void logFramingMode(String label, BenchResult r, long totalCells) String.format("%.2f", (double) r.appendNs / Math.max(1, totalCells))); } + @Test + public void testMutationAttributesRoundTrip() throws IOException { + long ts = 12345L; + Put put = new Put(Bytes.toBytes("row")); + put.setTimestamp(ts); + put.addColumn(Bytes.toBytes("cf"), Bytes.toBytes("q"), ts, Bytes.toBytes("v")); + put.setAttribute(PhoenixIndexCodec.INDEX_UUID, HConstants.EMPTY_BYTE_ARRAY); + put.setAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), + Bytes.toBytes("MY_SCHEMA")); + put.setAttribute(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + Bytes.toBytes("MY_TABLE")); + put.setAttribute(MutationState.MutationMetadataType.TENANT_ID.toString(), + Bytes.toBytes("tenant1")); + put.setAttribute(IndexRegionObserver.REPLICATED_MUTATION, HConstants.EMPTY_BYTE_ARRAY); + + LogFile.Record original = + new LogFileRecord().setHBaseTableName("TBLATTR").setCommitId(1L).setMutation(put); + + LogFileCodec codec = new LogFileCodec(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + codec.getEncoder(new DataOutputStream(baos)).write(original); + LogFile.Codec.Decoder decoder = + codec.getDecoder(new DataInputStream(new ByteArrayInputStream(baos.toByteArray()))); + + assertTrue(decoder.advance()); + Mutation decoded = decoder.current().getMutation(); + + for (String attrKey : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { + byte[] expected = put.getAttribute(attrKey); + byte[] actual = decoded.getAttribute(attrKey); + assertTrue("Attribute " + attrKey + " should be present", actual != null); + assertTrue("Attribute " + attrKey + " value should match", Arrays.equals(expected, actual)); + } + } + + @Test + public void testNoAttributesRoundTrip() throws IOException { + long ts = 12345L; + Put put = new Put(Bytes.toBytes("row")); + put.setTimestamp(ts); + put.addColumn(Bytes.toBytes("cf"), Bytes.toBytes("q"), ts, Bytes.toBytes("v")); + + LogFile.Record original = + new LogFileRecord().setHBaseTableName("TBLNOATTR").setCommitId(1L).setMutation(put); + + LogFileCodec codec = new LogFileCodec(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + codec.getEncoder(new DataOutputStream(baos)).write(original); + LogFile.Codec.Decoder decoder = + codec.getDecoder(new DataInputStream(new ByteArrayInputStream(baos.toByteArray()))); + + assertTrue(decoder.advance()); + Mutation decoded = decoder.current().getMutation(); + + for (String attrKey : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { + byte[] actual = decoded.getAttribute(attrKey); + assertTrue("Attribute " + attrKey + " should not be present", actual == null); + } + } + + @Test + public void testPartialAttributesRoundTrip() throws IOException { + long ts = 12345L; + Put put = new Put(Bytes.toBytes("row")); + put.setTimestamp(ts); + put.addColumn(Bytes.toBytes("cf"), Bytes.toBytes("q"), ts, Bytes.toBytes("v")); + put.setAttribute(PhoenixIndexCodec.INDEX_UUID, HConstants.EMPTY_BYTE_ARRAY); + put.setAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes("S")); + + LogFile.Record original = + new LogFileRecord().setHBaseTableName("TBLPARTATTR").setCommitId(1L).setMutation(put); + + LogFileCodec codec = new LogFileCodec(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + codec.getEncoder(new DataOutputStream(baos)).write(original); + LogFile.Codec.Decoder decoder = + codec.getDecoder(new DataInputStream(new ByteArrayInputStream(baos.toByteArray()))); + + assertTrue(decoder.advance()); + Mutation decoded = decoder.current().getMutation(); + + assertTrue("INDEX_UUID should be present", + decoded.getAttribute(PhoenixIndexCodec.INDEX_UUID) != null); + assertTrue("INDEX_UUID should be empty", + decoded.getAttribute(PhoenixIndexCodec.INDEX_UUID).length == 0); + assertTrue("SCHEMA_NAME should match", Arrays.equals(Bytes.toBytes("S"), + decoded.getAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString()))); + assertTrue("LOGICAL_TABLE_NAME should not be present", + decoded.getAttribute(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString()) + == null); + assertTrue("TENANT_ID should not be present", + decoded.getAttribute(MutationState.MutationMetadataType.TENANT_ID.toString()) == null); + } + } diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileTestUtil.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileTestUtil.java index a0a4e434422..cb2400325ac 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileTestUtil.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileTestUtil.java @@ -30,7 +30,6 @@ import java.io.EOFException; import java.io.IOException; import java.io.OutputStream; -import java.util.ArrayList; import java.util.List; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.FSDataOutputStreamBuilder; @@ -44,6 +43,7 @@ import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; import org.apache.hadoop.hbase.util.Bytes; +import org.apache.phoenix.replication.MutationCellGrouper; public interface LogFileTestUtil { @@ -51,11 +51,7 @@ public interface LogFileTestUtil { * Flatten a mutation's family cell map into the cell list that the writer ultimately receives. */ static List cellsOf(Mutation mutation) { - List cells = new ArrayList<>(); - for (List familyCells : mutation.getFamilyCellMap().values()) { - cells.addAll(familyCells); - } - return cells; + return MutationCellGrouper.flattenCells(mutation); } static LogFile.Record newPutRecord(String table, long commitId, String rowKey, long ts, From 2aed271507dc4180814eed33e3206e0564feee29 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Mon, 6 Jul 2026 15:08:12 -0700 Subject: [PATCH 2/6] Standby index: add table context to pre-image errors, doc notes Thread dataTableName into the standby pre-image decode/lookup so a contract violation names the offending table. decodePreImage and buildReplicatedRowGroups become instance methods reading the dataTableName field instead of taking it as a parameter; add a @VisibleForTesting IndexRegionObserver(String) constructor alongside the public no-arg one HBase loads reflectively. PreImageLocalTable takes the table name in its constructor. Doc notes: decodePreImage javadoc explains the schema-skew case as the one legitimate missing-pre-image path and why failing loud is correct; PreImageLocalTable and MutationCellGrouper get class-level thread-safety notes. --- .../hbase/index/IndexRegionObserver.java | 25 +++++++++++++++---- .../covered/data/PreImageLocalTable.java | 12 ++++++--- .../replication/MutationCellGrouper.java | 3 +++ .../index/IndexRegionObserverReplayTest.java | 18 +++++++------ 4 files changed, 42 insertions(+), 16 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 4b7bc8ef0f7..6d4130f0e40 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 @@ -652,6 +652,14 @@ private static Predicate getSynchronousReplicationFilter(byte[] tableN private Predicate ignoreReplicationFilter; + public IndexRegionObserver() { + } + + @VisibleForTesting + IndexRegionObserver(String dataTableName) { + this.dataTableName = dataTableName; + } + @Override public Optional getRegionObserver() { return Optional.of(this); @@ -1421,7 +1429,7 @@ static final class ReplicatedRowGroup { * calling this. */ @VisibleForTesting - static List buildReplicatedRowGroups(List enabledMutations) + List buildReplicatedRowGroups(List enabledMutations) throws IOException { LinkedHashMap> groups = new LinkedHashMap<>(); for (Mutation m : enabledMutations) { @@ -1536,13 +1544,20 @@ private void prepareReplicatedIndexMutations(MiniBatchOperationInProgress + * The one way this can legitimately occur is schema skew: the standby carries an index the active + * lacked when it shipped the batch, so the mutation is index-enabled on replay but no pre-image + * was captured on the active. That already breaks the feature's foundational assumption — index + * regeneration requires the active and standby to agree on the set of index maintainers — so we + * fail loud (a non-retryable {@link DoNotRetryIOException}) rather than silently regenerate the + * index against a missing prior state, which would corrupt it. */ @VisibleForTesting - static Put decodePreImage(Mutation m) throws IOException { + Put decodePreImage(Mutation m) throws IOException { byte[] preImageBytes = m.getAttribute(PRE_IMAGE); if (preImageBytes == null) { - throw new DoNotRetryIOException("Replicated mutation on indexed table is missing the " - + PRE_IMAGE + " attribute: row=" + Bytes.toStringBinary(m.getRow())); + throw new DoNotRetryIOException("Replicated mutation on table " + dataTableName + + " is missing the " + PRE_IMAGE + " attribute: row=" + Bytes.toStringBinary(m.getRow())); } if (preImageBytes.length == 0) { return null; @@ -2652,7 +2667,7 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) Collection mutations = buildReplayLocalIndexInputs(groups, preImageCellsByRowTs); handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, - new PreImageLocalTable(preImageCellsByRowTs)); + new PreImageLocalTable(dataTableName, preImageCellsByRowTs)); } else { Collection mutations = groupMutations(miniBatchOp, context); // dataRowStates is populated only by the global/uncovered/transform/atomic/... branch, so a diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java index 270acc8bce8..ffb176e7295 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/hbase/index/covered/data/PreImageLocalTable.java @@ -48,15 +48,21 @@ * A {@code null} cell list is the "active saw an empty row" sentinel and is the documented * {@link LocalHBaseState#getCurrentRowState} return for "no prior row". A key that is absent * entirely (never populated) is a contract violation and throws, so the sentinel stays unambiguous. + *

+ * Not thread-safe, and does not need to be: an instance is built per replay batch and consumed by + * that batch's single-threaded local-index build. The backing map is populated once at construction + * and never mutated afterward. */ public class PreImageLocalTable implements LocalHBaseState { /** Distinct marker for "key absent", so a stored null (empty-row sentinel) is not mistaken. */ private static final List ABSENT = Collections.emptyList(); + private final String dataTableName; private final Map> preImageCellsByRowTs; - public PreImageLocalTable(Map> preImageCellsByRowTs) { + public PreImageLocalTable(String dataTableName, Map> preImageCellsByRowTs) { + this.dataTableName = dataTableName; this.preImageCellsByRowTs = Preconditions.checkNotNull(preImageCellsByRowTs, "preImageCellsByRowTs must not be null"); } @@ -83,8 +89,8 @@ public List getCurrentRowState(Mutation mutation, // as the empty-row sentinel and silently regenerate the index against an empty prior state. List cells = preImageCellsByRowTs.getOrDefault(key, ABSENT); if (cells == ABSENT) { - throw new DoNotRetryIOException( - "No pre-image for replayed local-index row; (row, ts) key not populated: " + key); + throw new DoNotRetryIOException("No pre-image for replayed local-index row on table " + + dataTableName + "; (row, ts) key not populated: " + key); } return cells; } 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 a2c6e6d566d..adb071ff852 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 @@ -47,6 +47,9 @@ * recurs non-consecutively yields a separate mutation per run), not correctness -- cell order is * preserved, so replaying the resulting mutations in order reproduces the effect of applying the * input cells in order. + *

+ * Stateless and thread-safe: a non-instantiable holder of static methods that operate solely on + * their arguments. */ public final class MutationCellGrouper { diff --git a/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java b/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java index 3ae42cb36da..651eb54dde1 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/hbase/index/IndexRegionObserverReplayTest.java @@ -55,6 +55,8 @@ public class IndexRegionObserverReplayTest { private static final byte[] Q3 = Bytes.toBytes("c3"); private static final long TS = 100L; + private final IndexRegionObserver iro = new IndexRegionObserver("T"); + /** A Put on the given row at the given ts, one cell per qualifier. */ private static Put putRowTs(byte[] row, long ts, byte[]... qualifiers) { Put put = new Put(row); @@ -76,7 +78,7 @@ private static M withPreImage(M m, Put preImage) throws IOE public void testDecodePreImageMissingAttributeThrows() { Put m = putRowTs(ROW, TS, Q1); try { - IndexRegionObserver.decodePreImage(m); + iro.decodePreImage(m); fail("expected DoNotRetryIOException when PRE_IMAGE attribute is absent"); } catch (DoNotRetryIOException expected) { assertTrue(expected.getMessage().contains(IndexRegionObserver.PRE_IMAGE)); @@ -90,7 +92,7 @@ public void testDecodePreImageEmptySentinelReturnsNull() throws IOException { Put m = putRowTs(ROW, TS, Q1); m.setAttribute(IndexRegionObserver.PRE_IMAGE, HConstants.EMPTY_BYTE_ARRAY); assertNull("zero-length PRE_IMAGE is the 'active saw empty row' sentinel", - IndexRegionObserver.decodePreImage(m)); + iro.decodePreImage(m)); } @Test @@ -98,7 +100,7 @@ public void testDecodePreImageRoundTrip() throws IOException { Put preImage = putRowTs(ROW, TS, Q1, Q2); Put carrier = withPreImage(new Put(ROW), preImage); - Put decoded = IndexRegionObserver.decodePreImage(carrier); + Put decoded = iro.decodePreImage(carrier); assertTrue(decoded.has(CF, Q1)); assertTrue(decoded.has(CF, Q2)); assertTrue(CellUtil.matchingRow(decoded.getFamilyCellMap().get(CF).get(0), ROW)); @@ -203,7 +205,7 @@ public void testBuildReplicatedRowGroupsSplitsByTimestamp() throws IOException { Put g2 = withPreImage(putRowTs(ROW, 200L, Q2), putRowTs(ROW, 200L, Q1)); List groups = - IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(g1, g2)); + iro.buildReplicatedRowGroups(Arrays. asList(g1, g2)); assertEquals("two distinct (row, ts) groups", 2, groups.size()); assertEquals(100L, groups.get(0).ts); @@ -220,7 +222,7 @@ public void testBuildReplicatedRowGroupsEachGroupKeepsItsOwnPreImage() throws IO Put g2 = withPreImage(putRowTs(ROW, 200L, Q2), putRowTs(ROW, 200L, Q1)); List groups = - IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(g1, g2)); + iro.buildReplicatedRowGroups(Arrays. asList(g1, g2)); // group 1: no pre-image + a Put(Q1) -> next has Q1 assertTrue(groups.get(0).nextState.has(CF, Q1)); @@ -235,7 +237,7 @@ public void testBuildReplicatedRowGroupsMergesSameRowTs() throws IOException { Put b = withPreImage(putRowTs(ROW, 100L, Q2), null); List groups = - IndexRegionObserver.buildReplicatedRowGroups(Arrays. asList(a, b)); + iro.buildReplicatedRowGroups(Arrays. asList(a, b)); assertEquals("same (row, ts) collapses into one group", 1, groups.size()); assertEquals(2, groups.get(0).mutations.size()); @@ -264,8 +266,8 @@ public void testBuildReplicatedRowGroupsMultiRowMultiTsIsolation() throws IOExce // (R2, ts1): no pre-image; Put X adds Q1 -> next {Q1}. Put r2t1Put = withPreImage(putRowTs(R2, ts1, Q1), null); - List groups = IndexRegionObserver - .buildReplicatedRowGroups(Arrays. asList(r1t1Put, r1t1Del, r1t2Put, r2t1Put)); + List groups = + iro.buildReplicatedRowGroups(Arrays. asList(r1t1Put, r1t1Del, r1t2Put, r2t1Put)); // (a) exactly three groups, in first-seen order. assertEquals("three (row, ts) groups", 3, groups.size()); From a0197e9c24fd43fbf52832a83f5c5681b9157b48 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Tue, 7 Jul 2026 12:55:07 -0700 Subject: [PATCH 3/6] Standby index: carve a dedicated replication path in preBatchMutate preBatchMutateWithExceptions interleaved the active write path and the standby replay path in one method behind five scattered isReplication guards, so the standby path was implicit (defined by what active-only blocks skip). That fragility surfaced a real bug: getCurrentRowStates was gated only on index flags, not isReplication, so the standby scanned the data-table region under lock for prior row state -- the out-of-order-unsafe read the per-row PRE_IMAGE exists to replace -- and then discarded the result. - Skip getCurrentRowStates on replication: prepareReplicatedIndexMutations reads only the group's PRE_IMAGE/nextState, never dataRowStates, so the scan and its pendingRows/lastConcurrentBatchContext bookkeeping are inert on the standby. - Dispatch to a new preBatchMutateReplication right after currentPhase = PRE and return, so the active body runs only on the active. Drop the five now-dead isReplication guards there. - Extract the shared two-phase commit (preparePreIndexMutations -> unlock -> doPre -> lock -> [wait] -> post) into prepareAndCommitGlobalIndexUpdates so the lockstep protocol stays single-sourced; the active/standby fork lives inside preparePreIndexMutations. The concurrent-batch wait is now reached only when lastConcurrentBatchContext is set, which never happens on the standby. Verified: IndexRegionObserverReplayTest 16/16, ReplicationLogGroupIT 15/15 (incl. testConcurrentUpserts, concurrent same-row standby replay). --- .../hbase/index/IndexRegionObserver.java | 177 +++++++++++------- 1 file changed, 105 insertions(+), 72 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 6d4130f0e40..c4a3cb1c2f5 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 @@ -2560,8 +2560,17 @@ public void preBatchMutateWithExceptions(ObserverContext groups = getReplicatedRowGroups(miniBatchOp, context); - Map> preImageCellsByRowTs = new HashMap<>(); - Collection mutations = - buildReplayLocalIndexInputs(groups, preImageCellsByRowTs); - handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, - new PreImageLocalTable(dataTableName, preImageCellsByRowTs)); + // indexes). + Collection mutations = groupMutations(miniBatchOp, context); + // dataRowStates is populated only by the global/uncovered/transform/atomic/... branch, so a + // null map here means this table has only a local index and never ran the global-style + // pre-image capture. A non-null replicationCellsByRow means captureReplicationCells ran, + // i.e. this is a replicated active-side batch. Together they identify a replicated + // local-only table, which must ship a pre-image so the standby can regenerate its local + // index. Build the prior-state scan once and reuse it for both the pre-image capture and + // the index build. Mixed tables already captured a (superset) pre-image in the earlier + // branch; non-replicated local-only tables keep the unchanged path (no extra work). + if (context.dataRowStates == null && context.replicationCellsByRow != null) { + CachedLocalTable cachedLocalTable = + CachedLocalTable.build(mutations, indexMetaData, c.getEnvironment().getRegion()); + captureLocalIndexPreImageCells(miniBatchOp, context, mutations, cachedLocalTable); + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, cachedLocalTable); } else { - Collection mutations = groupMutations(miniBatchOp, context); - // dataRowStates is populated only by the global/uncovered/transform/atomic/... branch, so a - // null map here means this table has only a local index and never ran the global-style - // pre-image capture. A non-null replicationCellsByRow means captureReplicationCells ran, - // i.e. this is a replicated active-side batch. Together they identify a replicated - // local-only table, which must ship a pre-image so the standby can regenerate its local - // index. Build the prior-state scan once and reuse it for both the pre-image capture and - // the index build. Mixed tables already captured a (superset) pre-image in the earlier - // branch; non-replicated local-only tables keep the unchanged path (no extra work). - if (context.dataRowStates == null && context.replicationCellsByRow != null) { - CachedLocalTable cachedLocalTable = - CachedLocalTable.build(mutations, indexMetaData, c.getEnvironment().getRegion()); - captureLocalIndexPreImageCells(miniBatchOp, context, mutations, cachedLocalTable); - handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, cachedLocalTable); - } else { - handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, null); - } + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, null); } } if (failDataTableUpdatesForTesting) { @@ -2693,6 +2662,70 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) } } + /** + * Standby replay path for {@link #preBatchMutateWithExceptions}. Reached only for replicated + * batches (batches whose mutations carry the {@link #REPLICATED_MUTATION} marker), after the + * shared prologue has locked the rows and set the PRE phase. Deliberately runs none of the + * active-only steps: no timestamp assignment (cells already carry the active's final per-cell + * timestamps), no data-table row-state scan (index updates are regenerated from the per-row + * PRE_IMAGE the active shipped), no replication capture (a replayed batch is not re-replicated), + * and no concurrent-batch wait (each replicated batch is self-sufficient via its PRE_IMAGE, so + * concurrent standby batches on the same row need no ordering). + */ + private void preBatchMutateReplication(ObserverContext c, + MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context, + 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); + } + if (context.hasLocalIndex) { + // Group by (row, ts) so each replayed active-side batch's cells stay in their own uniform-ts + // mutation for NonTxIndexBuilder, and serve the builder's prior row state from each group's + // shipped PRE_IMAGE instead of a (not-yet-written) region scan. + List groups = getReplicatedRowGroups(miniBatchOp, context); + Map> preImageCellsByRowTs = new HashMap<>(); + Collection mutations = + buildReplayLocalIndexInputs(groups, preImageCellsByRowTs); + handleLocalIndexUpdates(table, miniBatchOp, mutations, indexMetaData, + new PreImageLocalTable(dataTableName, preImageCellsByRowTs)); + } + if (failDataTableUpdatesForTesting) { + throw new DoNotRetryIOException("Simulating the data table write failure"); + } + } + + /** + * Shared two-phase index-commit sequence for the global/uncovered/transform branch, run + * identically on the active and standby paths. The two paths differ only in how + * {@link #preparePreIndexMutations} fills {@code context.indexUpdates} (computed from + * {@code dataRowStates} on the active, regenerated from the per-row PRE_IMAGE on the standby); + * from there the prepare -> unlock -> doPre -> lock -> wait -> post protocol is the same. + */ + private void prepareAndCommitGlobalIndexUpdates(TableName table, + MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context, + long batchTimestamp, 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); + metricSource.updateIndexPrepareTime(dataTableName, + EnvironmentEdgeManager.currentTimeMillis() - start); + // Release the locks before making RPC calls for index updates + unlockRows(context); + // Do the first phase index updates + doPre(context); + // Acquire the locks again before letting the region proceed with data table updates + lockRows(context); + // Only populated by getCurrentRowStates, which the standby skips, so this is always null on + // replication: each replicated batch is self-sufficient via its PRE_IMAGE and needs no wait. + if (context.lastConcurrentBatchContext != null) { + waitForPreviousConcurrentBatch(table, context); + } + preparePostIndexMutations(context, indexMetaData); + } + /** * In case of ON DUPLICATE KEY IGNORE, if the row already exists no mutations will be generated so * release the row lock. From 1fcebd6eb9a01bf65cd0dd50e03fd45a31c6e3c7 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Tue, 7 Jul 2026 15:44:43 -0700 Subject: [PATCH 4/6] PHOENIX-7874: unify sync + WAL-restore replication on one local-index cell filter Both replication paths now drop local-index (L#) cells the same way. The WAL-restore path (replicateEditOnWALRestore) previously shipped the persisted WAL edit verbatim, leaking the L# cells HBase merged into the data mutation's family map -- a recovery-path-only leak no IT exercised. It now filters the cell stream through the shared isReplicableCell predicate. The synchronous path (replicateMutations) reads the batch's now-final cells from miniBatchOp in POST and drops L# cells with a single family-key check per family list, then appends the pre-image cells from WAL slot 0 (filtered through isReplicableCell). capturePreImageCells is now the sole producer of pre-image cells, written only to the WAL edit and read back by both paths, so they ship byte-identical output with no re-derivation. Removes the timing-based snapshot machinery (captureReplicationCells and the replicationCellsByRow field) that the sync path used to exclude L# cells, since the WAL-restore path could not use it. --- .../hbase/index/IndexRegionObserver.java | 223 +++++++++--------- 1 file changed, 105 insertions(+), 118 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 c4a3cb1c2f5..d7286955864 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 @@ -53,6 +53,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.function.Predicate; +import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hbase.Abortable; @@ -159,6 +160,7 @@ import org.apache.phoenix.util.EncodedColumnsUtil; import org.apache.phoenix.util.EnvironmentEdgeManager; import org.apache.phoenix.util.IndexUtil; +import org.apache.phoenix.util.MetaDataUtil; import org.apache.phoenix.util.MutationUtil; import org.apache.phoenix.util.PhoenixKeyValueUtil; import org.apache.phoenix.util.SchemaUtil; @@ -487,14 +489,6 @@ public static class BatchMutateContext { private boolean isReplication; // HAGroup associated with the batch private Optional logGroup = Optional.empty(); - // Per-row cell buckets captured in PRE phase that will be appended to the replication log in - // postBatchMutateIndispensably's replicateMutations. Captured unconditionally for every - // non-ignored mutation right after setTimestamps; later, when prepareDataRowStates runs (i.e. - // when the table has a global, uncovered, or transform index), the per-row pre-image cell is - // appended to the same row's bucket. Insertion order = batch order so cell ordering across - // rows is preserved on flatten. Null until captured. Replication attributes are NOT stored - // here; they are recomputed from originalMutations in replicateMutations. - private LinkedHashMap> replicationCellsByRow; // Per-(row, ts) groups folded from this replicated batch, computed once and shared by the // global-index path (prepareReplicatedIndexMutations) and the local-index path. Null until // first built; only populated on the standby replay path (isReplication). @@ -961,11 +955,12 @@ public void preWALRestore( } /** - * Forward the WAL edit's cell stream — including any METAFAMILY pre-image cells that were - * injected at PRE phase — to the replication log as a single batch. The standby reader peels the - * pre-image cells and reconstructs Put/Delete mutations on the way out. Ignored mutations were - * filtered out of {@code replicationCellsByRow} during {@code captureReplicationCells}, so the - * WAL edit already excludes them; no per-mutation filtering is required here. + * Forward the WAL edit's cell stream to the replication log as a single batch, filtered through + * {@link #isReplicableCell} — the same predicate the synchronous {@link #replicateMutations} path + * uses. The persisted WAL edit carries the local-index (L#) cells HBase merged into the data + * mutation's family map; those must be dropped (the standby regenerates its own local index), + * while the METAFAMILY pre-image cells injected at PRE phase are kept. The standby reader peels + * the pre-image cells and reconstructs Put/Delete mutations on the way out. * @param logGroup HA Group * @param logKey WAL log key * @param logEdit WAL edit record @@ -977,6 +972,15 @@ private void replicateEditOnWALRestore(ReplicationLogGroup logGroup, WALKey logK if (cells == null || cells.isEmpty()) { return; } + // The persisted WAL edit carries the local-index (L#) cells HBase merged into the data + // mutation's family map, plus our METAFAMILY pre-image cells and possibly foreign coprocessor + // markers. Filter through the same predicate the synchronous path uses so both ship exactly the + // data cells and our pre-image, and never a local-index cell (the standby regenerates its own). + List replicable = + cells.stream().filter(IndexRegionObserver::isReplicableCell).collect(Collectors.toList()); + if (replicable.isEmpty()) { + return; + } Map replicationAttrs = new HashMap<>(); for (String attrKey : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { byte[] val = walKeyAttrs.get(attrKey); @@ -984,7 +988,7 @@ private void replicateEditOnWALRestore(ReplicationLogGroup logGroup, WALKey logK replicationAttrs.put(attrKey, val); } } - logGroup.append(tableName, -1, cells, replicationAttrs); + logGroup.append(tableName, -1, replicable, replicationAttrs); logGroup.sync(); } @@ -1650,62 +1654,26 @@ static Put deriveNextState(Put preImage, List groupMutations) throws I } /** - * Capture the cell stream that will be appended to the replication log. - *

- * Called unconditionally from {@link #preBatchMutateWithExceptions} after {@code setTimestamps} - * so cells carry their final timestamps and any post-on-dup / post-conditional-TTL cells from - * earlier phases are reflected. Every Phoenix-write batch ships through Phoenix replication and - * lands in the standby's {@code preBatchMutate}, regardless of which indexes the active table - * has. Tables with global/uncovered/transform indexes get an additional pre-image cell appended - * later by {@link #capturePreImageCells} (called from {@link #prepareDataRowStates}); other - * tables ship data cells only and the standby IRO falls through to its primary code path. - *

- * Each non-ignored mutation contributes its family cells to its row's bucket in - * {@code context.replicationCellsByRow}. CP-injected mutations visible at this call site (on-dup - * Deletes from {@link #addOnDupMutationsToBatch}, conditional-TTL masking Deletes from - * {@link #updateMutationsForConditionalTTL}) are appended to the same row's bucket. Local-index - * updates are NOT visible here: {@link #handleLocalIndexUpdates} runs later via - * {@code addOperationsFromCP(0, …)} so they correctly stay out of the replication log. - *

- * The map's key set is the set of replicated rows (no separate set field needed); a row whose - * only mutation was filtered by {@code ignoreReplicationFilter} does not appear in the map. + * 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). */ - private void captureReplicationCells(MiniBatchOperationInProgress miniBatchOp, - BatchMutateContext context) { - if (!shouldReplicate || ignoreSyncReplicationForTesting) { - return; - } - if (!context.logGroup.isPresent()) { - return; - } - LinkedHashMap> cellsByRow = new LinkedHashMap<>(); - for (int i = 0; i < miniBatchOp.size(); i++) { - Mutation m = miniBatchOp.getOperation(i); - if (ignoreReplicationFilter.test(m)) { - continue; - } - ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(m.getRow()); - List bucket = cellsByRow.computeIfAbsent(rowKeyPtr, k -> new ArrayList<>()); - for (List cells : m.getFamilyCellMap().values()) { - bucket.addAll(cells); - } - Mutation[] cpMutations = miniBatchOp.getOperationsFromCoprocessors(i); - if (cpMutations != null) { - for (Mutation cp : cpMutations) { - for (List cells : cp.getFamilyCellMap().values()) { - bucket.addAll(cells); - } - } - } - } - context.replicationCellsByRow = cellsByRow; + private boolean isReplicatedBatch(BatchMutateContext context) { + return shouldReplicate && !ignoreSyncReplicationForTesting && context.logGroup.isPresent(); } /** - * Emit one pre-image cell per replicated row that has a {@code dataRowStates} entry. Appends the - * cell to the row's bucket in {@code context.replicationCellsByRow} and to - * {@code miniBatchOp.getWalEdit(0)} so the WAL-restore path ships the same payload as the - * synchronous {@link #replicateMutations} call. + * Emit one pre-image cell per replicated row into {@code miniBatchOp.getWalEdit(0)}. This is the + * sole producer of pre-image cells for both replication paths: the synchronous + * {@link #replicateMutations} reads them back from the same WAL slot in POST, and the WAL-restore + * {@link #replicateEditOnWALRestore} reads them from the persisted WAL edit HBase builds from + * that slot — so both ship byte-identical pre-images with no re-derivation. + *

+ * We walk the batch (not {@code dataRowStates} directly) and skip {@code ignoreReplicationFilter} + * mutations, so a row filtered out of replication (e.g. syscat rows without a leading tenant id) + * gets no pre-image even though it may carry a {@code dataRowStates} entry. Each replicated row + * is emitted once, from its {@code dataRowStates} entry. *

* Pre-image bytes are PB-encoded {@link Put}; an empty value is the sentinel for "primary * observed an empty row at lock time" so the standby can distinguish that from "no primary @@ -1713,29 +1681,33 @@ private void captureReplicationCells(MiniBatchOperationInProgress mini * pre-image cell — the standby falls back to its no-pre-image code path for those rows. *

* Called from {@link #preBatchMutateWithExceptions} immediately after - * {@link #prepareDataRowStates} returns, on the global/uncovered/transform-index branch only. For - * tables without such indexes, {@code captureReplicationCells} alone produces the cell stream (no - * pre-image cells). + * {@link #prepareDataRowStates} returns, on the global/uncovered/transform-index branch, and from + * {@link #captureLocalIndexPreImageCells} on the local-only replicated branch. */ private void capturePreImageCells(MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) throws IOException { - if (context.replicationCellsByRow == null || context.replicationCellsByRow.isEmpty()) { + if (context.dataRowStates == null || context.dataRowStates.isEmpty()) { return; } WALEdit walEdit = miniBatchOp.getWalEdit(0); if (walEdit == null) { walEdit = new WALEdit(); } - for (Map.Entry> entry : context.replicationCellsByRow - .entrySet()) { - ImmutableBytesPtr rowKeyPtr = entry.getKey(); + Set emitted = new HashSet<>(); + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (ignoreReplicationFilter.test(m)) { + continue; + } + ImmutableBytesPtr rowKeyPtr = new ImmutableBytesPtr(m.getRow()); + if (!emitted.add(rowKeyPtr)) { + continue; + } Pair rowState = context.dataRowStates.get(rowKeyPtr); if (rowState == null) { continue; } - Cell preImageCell = buildPreImageCell(rowKeyPtr.copyBytes(), rowState.getFirst()); - entry.getValue().add(preImageCell); - walEdit.add(preImageCell); + walEdit.add(buildPreImageCell(rowKeyPtr.copyBytes(), rowState.getFirst())); } if (!walEdit.isEmpty()) { miniBatchOp.setWalEdit(0, walEdit); @@ -1757,9 +1729,6 @@ private void capturePreImageCells(MiniBatchOperationInProgress miniBat private void captureLocalIndexPreImageCells(MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context, Collection groupedMutations, CachedLocalTable cachedLocalTable) throws IOException { - if (context.replicationCellsByRow == null || context.replicationCellsByRow.isEmpty()) { - return; - } context.dataRowStates = new HashMap<>(groupedMutations.size()); for (Mutation m : groupedMutations) { List priorCells = @@ -2623,16 +2592,15 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) // 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)); - // Snapshot the cell stream for replication after timestamps are final, before the index - // branches run. The pre-image cells are appended later, after prepareDataRowStates populates - // dataRowStates. - captureReplicationCells(miniBatchOp, context); if (context.hasGlobalIndex || context.hasUncoveredIndex || context.hasTransform) { // Prepare next data rows states for pending mutations (for global indexes). prepareDataRowStates(c, miniBatchOp, context, batchTimestamp); - // dataRowStates is now populated; append per-row pre-image cells to the captured buckets - // and to the WAL edit so the WAL-restore path ships the same payload as replicateMutations. - capturePreImageCells(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); } @@ -2642,13 +2610,12 @@ && isPartialUncoveredIndexMutation(indexMetaData, miniBatchOp) Collection mutations = groupMutations(miniBatchOp, context); // dataRowStates is populated only by the global/uncovered/transform/atomic/... branch, so a // null map here means this table has only a local index and never ran the global-style - // pre-image capture. A non-null replicationCellsByRow means captureReplicationCells ran, - // i.e. this is a replicated active-side batch. Together they identify a replicated + // pre-image capture. Combined with a replicated batch, that identifies a replicated // local-only table, which must ship a pre-image so the standby can regenerate its local // index. Build the prior-state scan once and reuse it for both the pre-image capture and // the index build. Mixed tables already captured a (superset) pre-image in the earlier // branch; non-replicated local-only tables keep the unchanged path (no extra work). - if (context.dataRowStates == null && context.replicationCellsByRow != null) { + if (context.dataRowStates == null && isReplicatedBatch(context)) { CachedLocalTable cachedLocalTable = CachedLocalTable.build(mutations, indexMetaData, c.getEnvironment().getRegion()); captureLocalIndexPreImageCells(miniBatchOp, context, mutations, cachedLocalTable); @@ -2890,7 +2857,9 @@ public void postBatchMutateIndispensably(ObserverContext postIndexFuture = CompletableFuture.runAsync(() -> doPost(c, context)); - replicateMutations(c.getEnvironment(), miniBatchOp, context); + if (isReplicatedBatch(context)) { + replicateMutations(context.logGroup.get(), miniBatchOp, context); + } FutureUtils.get(postIndexFuture); } } finally { @@ -3444,15 +3413,28 @@ public static boolean isAtomicOperationComplete(OperationStatus status) { return status.getOperationStatusCode() == SUCCESS && status.getResult() != null; } - private void replicateMutations(RegionCoprocessorEnvironment env, + /** + * A cell crosses the replication wire iff it is data to replicate or our own pre-image marker. + * Local-index (L#) cells are dropped: the standby regenerates its local index from the data + * record, and a replicated L# rowkey would carry the active region's start key, meaningless on + * the standby whose regions are split and assigned independently. METAFAMILY cells are dropped + * unless they carry our {@link #PRE_IMAGE_WAL_QUALIFIER}, so a foreign coprocessor's WAL + * contribution (or any other HBase system marker) cannot leak onto the wire. Applied identically + * by the synchronous ({@link #replicateMutations}) and WAL-restore + * ({@link #replicateEditOnWALRestore}) paths so both enforce one wire invariant: data plus our + * pre-image only. + */ + private static boolean isReplicableCell(Cell c) { + if (CellUtil.matchingFamily(c, WALEdit.METAFAMILY)) { + return CellUtil.matchingQualifier(c, PRE_IMAGE_WAL_QUALIFIER); + } + return !MetaDataUtil.isLocalIndexFamily( + new ImmutableBytesPtr(c.getFamilyArray(), c.getFamilyOffset(), c.getFamilyLength())); + } + + private void replicateMutations(ReplicationLogGroup logGroup, MiniBatchOperationInProgress miniBatchOp, BatchMutateContext context) throws IOException { - if (!shouldReplicate || ignoreSyncReplicationForTesting) { - return; - } - if (!context.logGroup.isPresent()) { - return; - } // Replicated batches on the standby never re-replicate. if (context.isReplication) { return; @@ -3460,34 +3442,39 @@ private void replicateMutations(RegionCoprocessorEnvironment env, if (context.getOriginalMutations().isEmpty()) { return; } + // Read the batch's now-final cells directly from miniBatchOp in POST. By this point HBase has + // finalized timestamps and merged every coprocessor-added cell (local index, on-dup, TTL) into + // the data mutation's family map (checkAndMergeCPMutations). Local-index (L#) cells are merged + // under their own L# family, so a single family-key check drops the whole list without touching + // each cell. The pre-image cells live in the WAL edit (slot 0), written by + // capturePreImageCells; + // they are filtered through isReplicableCell so a foreign coprocessor's slot-0 contribution + // cannot leak onto the wire. List flattened = new ArrayList<>(); - if (context.replicationCellsByRow != null && !context.replicationCellsByRow.isEmpty()) { - // Indexed path: cells were captured in the PRE phase right after setTimestamps, with the - // per-row pre-image sidecar cells appended. Ship the captured stream as-is. - for (List bucket : context.replicationCellsByRow.values()) { - flattened.addAll(bucket); + for (int i = 0; i < miniBatchOp.size(); i++) { + Mutation m = miniBatchOp.getOperation(i); + if (ignoreReplicationFilter.test(m)) { + continue; } - } else { - // No-index path: preBatchMutateWithExceptions short-circuits at the rowsToLock.isEmpty() - // early-exit before captureReplicationCells runs. setTimestamps is a no-op for such tables, - // so the cells still carry LATEST_TIMESTAMP in PRE; HBase finalizes them only after - // preBatchMutate returns. Read the now-final cells directly from miniBatchOp here in POST. - // There are no indexes (so no pre-image cells) and no CP-injected mutations (those come from - // atomic/ON-DUP handling, which would have populated rowsToLock and skipped the early-exit). - for (int i = 0; i < miniBatchOp.size(); i++) { - Mutation m = miniBatchOp.getOperation(i); - if (ignoreReplicationFilter.test(m)) { - continue; + for (Map.Entry> entry : m.getFamilyCellMap().entrySet()) { + // Drop L# cells: the standby regenerates its own local index from the data record, and a + // replicated L# rowkey would carry the active region's start key, which is meaningless on + // the standby whose regions are split and assigned independently. + if (!MetaDataUtil.isLocalIndexFamily(entry.getKey())) { + flattened.addAll(entry.getValue()); } - flattened.addAll(MutationCellGrouper.flattenCells(m)); - } - if (flattened.isEmpty()) { - return; } } + WALEdit preImageEdit = miniBatchOp.getWalEdit(0); + if (preImageEdit != null) { + preImageEdit.getCells().stream().filter(IndexRegionObserver::isReplicableCell) + .forEach(flattened::add); + } + if (flattened.isEmpty()) { + return; + } Map replicationAttributes = MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0)); - ReplicationLogGroup logGroup = context.logGroup.get(); logGroup.append(dataTableName, -1, flattened, replicationAttributes); logGroup.sync(); } From 52f13ea54004d7c453e072a7a003b45e9ec79e27 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Wed, 15 Jul 2026 11:53:37 -0700 Subject: [PATCH 5/6] Test: pin non-indexed table envelope omits INDEX_UUID extractReplicationAttributes must not invent an empty INDEX_UUID for a table without indexes -- an empty UUID would push the standby down the server-cache resolution branch and fail with INDEX_METADATA_NOT_FOUND instead of recognizing the table as non-indexed. Existing coverage only exercised the UUID-present and zero-attribute cases; this locks the contract for a mutation carrying schema/table metadata but no UUID. --- .../replication/MutationCellGrouperTest.java | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java index 08e1c3e081d..ef6f7bc220e 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java @@ -391,6 +391,30 @@ public void testExtractReplicationAttributesEmptyForBareMutation() throws Except assertTrue(extracted.isEmpty()); } + /** + * A non-indexed table's write carries the schema/table/tenant metadata attributes but no + * {@link PhoenixIndexCodec#INDEX_UUID} (the client only sets INDEX_UUID when index metadata is + * present). The extracted envelope must likewise omit INDEX_UUID: an empty UUID here would push + * the standby down the server-cache resolution branch and fail with INDEX_METADATA_NOT_FOUND + * instead of recognizing the table as non-indexed. + */ + @Test + public void testExtractReplicationAttributesOmitsIndexUuidForNonIndexedTable() throws Exception { + Put p = new Put(Bytes.toBytes("r")); + p.setAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes("S")); + p.setAttribute(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + Bytes.toBytes("T")); + + Map extracted = MutationCellGrouper.extractReplicationAttributes(p); + + assertNull("a non-indexed table's mutation carries no INDEX_UUID, so the envelope must not " + + "invent one", extracted.get(PhoenixIndexCodec.INDEX_UUID)); + assertArrayEquals(Bytes.toBytes("S"), + extracted.get(MutationState.MutationMetadataType.SCHEMA_NAME.toString())); + assertArrayEquals(Bytes.toBytes("T"), + extracted.get(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString())); + } + // ---------- contiguity stress (pre-image cell never breaks groupable runs) ---------- @Test From 06e8aca18f8a5e9635ebe56a2ee2ab2967ddd1f2 Mon Sep 17 00:00:00 2001 From: tkhurana Date: Wed, 15 Jul 2026 12:52:41 -0700 Subject: [PATCH 6/6] PHOENIX-7874: stamp INDEX_UUID from active's resolved maintainers, not client attr Remove INDEX_UUID from REPLICATION_ATTR_KEYS and stop normalizing it in extractReplicationAttributes. Whether the standby regenerates indexes is now decided by the active from its own resolved index maintainers (BatchMutateContext.hasIndex()), which stamps an empty INDEX_UUID only for indexed tables via the new MutationCellGrouper.stampIndexAttribute utility. Both the synchronous (replicateMutations) and WAL-restore paths route through buildReplicationAttributes so a non-indexed table never ships an empty UUID that would fail on the standby with INDEX_METADATA_NOT_FOUND. --- .../hbase/index/IndexRegionObserver.java | 40 ++++++++++++++++-- .../replication/MutationCellGrouper.java | 41 +++++++------------ .../replication/ReplicationLogGroup.java | 12 ++++-- .../reader/ReplicationLogProcessorTestIT.java | 9 +++- .../replication/MutationCellGrouperTest.java | 8 ++-- .../replication/log/LogFileCodecTest.java | 8 ++-- 6 files changed, 75 insertions(+), 43 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 d7286955864..23c0ef3fd30 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 @@ -132,7 +132,9 @@ import org.apache.phoenix.hbase.index.write.LazyParallelWriterIndexCommitter; 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.query.KeyRange; import org.apache.phoenix.query.QueryConstants; @@ -552,6 +554,11 @@ public void countDownAllLatches() { public int getMaxPendingRowCount() { return maxPendingRowCount; } + + /** True if the batch's table carries any index the standby must regenerate. */ + public boolean hasIndex() { + return hasGlobalIndex || hasUncoveredIndex || hasLocalIndex || hasTransform; + } } private ThreadLocal batchMutateContext = @@ -988,6 +995,13 @@ private void replicateEditOnWALRestore(ReplicationLogGroup logGroup, WALKey logK 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); + } logGroup.append(tableName, -1, replicable, replicationAttrs); logGroup.sync(); } @@ -2788,8 +2802,7 @@ private void appendReplicationAttributesToWALKey(WALKey key, if (context == null || context.getOriginalMutations().isEmpty()) { return; } - Map replicationAttributes = - MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0)); + Map replicationAttributes = buildReplicationAttributes(context); for (Map.Entry e : replicationAttributes.entrySet()) { IndexRegionObserver.appendToWALKey(key, e.getKey(), e.getValue()); } @@ -3473,9 +3486,28 @@ private void replicateMutations(ReplicationLogGroup logGroup, if (flattened.isEmpty()) { return; } - Map replicationAttributes = - MutationCellGrouper.extractReplicationAttributes(context.getOriginalMutations().get(0)); + Map replicationAttributes = buildReplicationAttributes(context); logGroup.append(dataTableName, -1, flattened, replicationAttributes); logGroup.sync(); } + + /** + * 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 buildReplicationAttributes(BatchMutateContext context) { + Map 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 adb071ff852..210e32e63d2 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 @@ -19,7 +19,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -29,9 +28,7 @@ import org.apache.hadoop.hbase.client.Delete; import org.apache.hadoop.hbase.client.Mutation; import org.apache.hadoop.hbase.client.Put; -import org.apache.hadoop.hbase.util.Bytes; import org.apache.hadoop.hbase.wal.WALEdit; -import org.apache.phoenix.execute.MutationState; import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; import org.apache.phoenix.index.PhoenixIndexCodec; @@ -75,27 +72,24 @@ public static List flattenCells(Mutation mutation) { /** * Extract the well-known replication attributes - * ({@link ReplicationLogGroup#REPLICATION_ATTR_KEYS}) from the mutation. Returns an empty map if - * the mutation has no attributes or none match. + * ({@link ReplicationLogGroup#REPLICATION_ATTR_KEYS}) from the mutation, copied verbatim. Returns + * an empty (mutable) map if the mutation has no attributes or none match. *

- * {@link PhoenixIndexCodec#INDEX_UUID}, when present, is always written as an empty byte array - * regardless of its value on the mutation. A non-empty UUID is a server-cache key scoped to the - * active cluster's region servers and is meaningless on the standby (it would fail to resolve - * with INDEX_METADATA_NOT_FOUND). An empty UUID forces the standby down the server-PTable - * resolution path, which rebuilds index maintainers from the schema/table/tenant attributes - * carried in this same envelope. + * {@link PhoenixIndexCodec#INDEX_UUID} is deliberately NOT part of this envelope: whether the + * standby regenerates indexes is decided by the active from its resolved index maintainers, not + * from the client-set UUID attribute. The active stamps an empty UUID onto the returned map only + * for indexed tables (see {@code IndexRegionObserver}). */ public static Map extractReplicationAttributes(Mutation mutation) { + Map envelope = new HashMap<>(); Map mutationAttrs = mutation.getAttributesMap(); if (mutationAttrs == null || mutationAttrs.isEmpty()) { - return Collections.emptyMap(); + return envelope; } - Map envelope = new HashMap<>(); for (String key : ReplicationLogGroup.REPLICATION_ATTR_KEYS) { byte[] v = mutationAttrs.get(key); if (v != null) { - envelope.put(key, - PhoenixIndexCodec.INDEX_UUID.equals(key) ? HConstants.EMPTY_BYTE_ARRAY : v); + envelope.put(key, v); } } return envelope; @@ -121,19 +115,14 @@ public static List buildReplicatedCells(List mutations, Put preI } /** - * Build the replication attribute envelope that rides alongside a {@link #buildReplicatedCells} - * cell stream: an empty {@link PhoenixIndexCodec#INDEX_UUID} (forcing the standby down the - * server-PTable resolution path, see {@link #extractReplicationAttributes}) plus the schema and - * logical table names the standby uses to rebuild index maintainers. + * Stamp an empty {@link PhoenixIndexCodec#INDEX_UUID} onto a replication attribute envelope. An + * empty UUID forces the standby down the server-PTable resolution path, which rebuilds index + * maintainers from the schema/table/tenant attributes in the same envelope. Callers apply this + * only for indexed tables (a non-indexed table needs no regeneration, and an empty UUID there + * would fail on the standby with INDEX_METADATA_NOT_FOUND). */ - public static Map buildReplicationAttributes(String schemaName, - String logicalTableName) { - Map attrs = new HashMap<>(); + public static void stampIndexAttribute(Map attrs) { attrs.put(PhoenixIndexCodec.INDEX_UUID, HConstants.EMPTY_BYTE_ARRAY); - attrs.put(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), Bytes.toBytes(schemaName)); - attrs.put(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), - Bytes.toBytes(logicalTableName)); - return attrs; } /** diff --git a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java index 6329c4b317e..9959ecc8aab 100644 --- a/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java +++ b/phoenix-core-server/src/main/java/org/apache/phoenix/replication/ReplicationLogGroup.java @@ -65,7 +65,6 @@ import org.apache.hadoop.hbase.ServerName; import org.apache.hadoop.hbase.client.Mutation; import org.apache.phoenix.execute.MutationState; -import org.apache.phoenix.index.PhoenixIndexCodec; import org.apache.phoenix.jdbc.HAGroupStoreManager; import org.apache.phoenix.jdbc.HAGroupStoreRecord; import org.apache.phoenix.jdbc.HAGroupStoreRecord.HAGroupState; @@ -196,10 +195,15 @@ public class ReplicationLogGroup { * {@code PRE_IMAGE} (per-row primary-side pre-image bytes) are intentionally NOT in this list. * Both are reader-synthesized: the standby stamps {@code REPLICATED_MUTATION} on every * reconstructed mutation, and {@code PRE_IMAGE} on those whose row had a pre-image cell. + *

+ * {@code INDEX_UUID} is also NOT in this list. It is not copied from the mutation: the active + * decides from its own resolved index maintainers whether the table is indexed and, only then, + * stamps an empty UUID onto the envelope (see {@code IndexRegionObserver}). Keying off the + * client-set attribute here would make the standby's index regeneration depend on client behavior + * rather than on whether the table actually has indexes. */ - public static final List REPLICATION_ATTR_KEYS = - Collections.unmodifiableList(Arrays.asList(PhoenixIndexCodec.INDEX_UUID, - MutationState.MutationMetadataType.SCHEMA_NAME.toString(), + public static final List REPLICATION_ATTR_KEYS = Collections + .unmodifiableList(Arrays.asList(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), MutationState.MutationMetadataType.TENANT_ID.toString())); 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 95b34ec6a3e..089bde4d473 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 @@ -70,6 +70,7 @@ import org.apache.hadoop.hbase.util.Pair; import org.apache.phoenix.end2end.NeedsOwnMiniClusterTest; import org.apache.phoenix.end2end.ParallelStatsDisabledIT; +import org.apache.phoenix.execute.MutationState; import org.apache.phoenix.hbase.index.IndexRegionObserver; import org.apache.phoenix.jdbc.PhoenixConnection; import org.apache.phoenix.query.QueryServices; @@ -2252,7 +2253,13 @@ public void testOutOfOrderReplayRegeneratesConsistentIndex() throws Exception { private void appendReplicatedBatch(Path file, String tableName, List mutations, Put preImage) throws IOException { List cells = MutationCellGrouper.buildReplicatedCells(mutations, preImage); - Map attrs = MutationCellGrouper.buildReplicationAttributes("", tableName); + Mutation attrCarrier = mutations.get(0); + attrCarrier.setAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString(), + Bytes.toBytes("")); + attrCarrier.setAttribute(MutationState.MutationMetadataType.LOGICAL_TABLE_NAME.toString(), + Bytes.toBytes(tableName)); + Map attrs = MutationCellGrouper.extractReplicationAttributes(attrCarrier); + MutationCellGrouper.stampIndexAttribute(attrs); LogFileWriter writer = initLogFileWriter(file); try { writer.append(tableName, -1, cells, attrs); diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java index ef6f7bc220e..96dc60c0d01 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/MutationCellGrouperTest.java @@ -370,10 +370,10 @@ public void testExtractReplicationAttributesFiltersToWellKnownKeys() throws Exce Map extracted = MutationCellGrouper.extractReplicationAttributes(p); - assertArrayEquals( - "INDEX_UUID must be normalized to empty so the standby resolves index " - + "maintainers from its own PTable rather than the active's server-cache key", - HConstants.EMPTY_BYTE_ARRAY, extracted.get(PhoenixIndexCodec.INDEX_UUID)); + assertNull( + "INDEX_UUID is not part of the extracted envelope; the active stamps an empty UUID " + + "itself, gated on its resolved index maintainers", + extracted.get(PhoenixIndexCodec.INDEX_UUID)); assertArrayEquals(Bytes.toBytes("S"), extracted.get(MutationState.MutationMetadataType.SCHEMA_NAME.toString())); assertNull("non-replication attribute must be filtered out", diff --git a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java index 2febc5a4267..c3dfb162d3b 100644 --- a/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java +++ b/phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileCodecTest.java @@ -828,10 +828,10 @@ public void testPartialAttributesRoundTrip() throws IOException { assertTrue(decoder.advance()); Mutation decoded = decoder.current().getMutation(); - assertTrue("INDEX_UUID should be present", - decoded.getAttribute(PhoenixIndexCodec.INDEX_UUID) != null); - assertTrue("INDEX_UUID should be empty", - decoded.getAttribute(PhoenixIndexCodec.INDEX_UUID).length == 0); + assertTrue( + "INDEX_UUID is not part of the replication envelope, so a UUID set on the mutation " + + "must not round-trip through the record", + decoded.getAttribute(PhoenixIndexCodec.INDEX_UUID) == null); assertTrue("SCHEMA_NAME should match", Arrays.equals(Bytes.toBytes("S"), decoded.getAttribute(MutationState.MutationMetadataType.SCHEMA_NAME.toString()))); assertTrue("LOGICAL_TABLE_NAME should not be present",