CEP-45: Incremental repair for mutation tracking by aweisberg · Pull Request #4696 · apache/cassandra

aweisberg · 2026-03-27T19:59:39Z

No description provided.

…on-atomically

…dn't work in 2 minutes it won't work.

…tionTrackingIncrementalRepairTask

…repair

…ce shard for keyspace distributed_test_keyspace, but it already exists on bounce

…tables

…marking sstables repaired to effect the migration

…e it will hang, so just use IR instead

…ntirely inside migrated ranges or entirely outside, but not both

…een completed, use tryFailure instead

maedhroz · 2026-04-01T19:38:55Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+            catch (RuntimeException e)
+            {
+                allSucceeded = false;
+                error = Throwables.merge(error, e);


nit: Coverage tooling indicates this might not be tested.

I'll convert timeouts to exceptions so it can be exercised.

maedhroz · 2026-04-01T19:39:05Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+            catch (Exception e)
+            {
+                logger.error("Error during mutation tracking repair", e);
+                resultPromise.tryFailure(e);


nit: Coverage tooling indicates this might not be tested.

The errors are put in resultPromise and don't get surfaced by allowing them to bubble up. To make this branch fire I could let exceptions bubble up and then get handled here. Would be less exception handling in general and then it would show up as tested.

I'll do that.

maedhroz · 2026-04-01T19:39:14Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+        if (allRanges.isEmpty())
+        {
+            logger.info("No common ranges to repair for keyspace {}", keyspace);
+            return new AsyncPromise<CoordinatedRepairResult>().setSuccess(CoordinatedRepairResult.create(List.of(), List.of()));


nit: Coverage tooling indicates this might not be tested.

maedhroz · 2026-04-01T19:39:54Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+        if (overlappingShards.isEmpty())
+        {
+            completionFuture.setSuccess(null);
+            return;


nit: Coverage tooling indicates this might not be tested.

Converted to a checkState

maedhroz · 2026-04-01T19:40:13Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+                                                     public void onFailure(InetAddressAndPort from, RequestFailure failure)
+                                                     {
+                                                         fail(new RuntimeException(
+                                                             String.format("Mutation tracking sync failed: participant %s returned failure %s", from, failure.reason)));


nit: Coverage tooling indicates this might not be tested.

Added a test case that sends an exception through here

maedhroz · 2026-04-01T19:40:33Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+            Shard currentShard = getCurrentShard(state.shard.range);
+            if (currentShard != state.shard)
+            {
+                failWithTopologyChange();


nit: Coverage tooling indicates this might not be tested.

Topology changes aren't supported yet https://issues.apache.org/jira/browse/CASSANDRA-20386

I'll take a look and see if I can at least induce one to exercise this failure path.

It might end up being more unit test then end to end test.

Added an end to end test. Seems like we don't error out on topology changes and more or less do it.

maedhroz · 2026-04-01T19:41:56Z

src/java/org/apache/cassandra/repair/messages/MutationTrackingSyncRequest.java

+ * their current witnessed offsets. This establishes a happens-before relationship: the
+ * participant's response contains offsets captured after receiving this request, which is
+ * sent after the repair starts.
+ *


nit:

Suggested change

*

* <p>

maedhroz · 2026-04-01T20:36:24Z

src/java/org/apache/cassandra/io/sstable/format/SSTableWriter.java

+                inMigrationPendingRange = migrationInfo.isRangeInPendingMigration(metadata().id,
+                                                                                   first.getToken(),
+                                                                                   last.getToken());
+            }


nit: Could replace the above w/

KeyspaceMigrationInfo migrationInfo = ClusterMetadata.current().mutationTrackingMigrationState.getKeyspaceInfo(metadata().keyspace); boolean inMigrationPendingRange = migrationInfo != null && migrationInfo.isRangeInPendingMigration(metadata().id, first.getToken(), last.getToken());

maedhroz · 2026-04-01T20:46:10Z

src/java/org/apache/cassandra/db/lifecycle/Tracker.java

+        // when incremental repair streams SSTables that were written before tracking was enabled.
+        Preconditions.checkState(!cfstore.metadata().replicationType().isTracked()
+                                 || ClusterMetadata.current().mutationTrackingMigrationState
+                                    .getKeyspaceInfo(cfstore.metadata().keyspace) != null);


nit: Might be nice to have something like an isMigrating(String) on MTMS, but just a matter of taste I guess.

I'll update it to use a helper.

maedhroz · 2026-04-01T20:46:49Z

src/java/org/apache/cassandra/db/lifecycle/Tracker.java

    {
-        Preconditions.checkState(!cfstore.metadata().replicationType().isTracked());
+        // Tracked tables may legitimately use this path during migration from untracked to tracked,
+        // when incremental repair streams SSTables that were written before tracking was enabled.


Does this mean that during migration to tracked, we'd expect these SSTables to have no coordinator log offsets then? Is that worth asserting?

It shouldn't matter for imports, since the keyspace being currently tracked means we'll avoid this method.

No we will actually hit this method during migration. The sstables might actually have offsets in them since tracked writes have already started and the incremental repair starts after.

maedhroz · 2026-04-02T16:38:46Z

src/java/org/apache/cassandra/db/AbstractMutationVerbHandler.java

+        // flag on the mutation hasn't been set yet at this point — it's set later in
+        // applyMutation() — so we check the handler type instead.
+        if (this instanceof ReadRepairVerbHandler)
+            return metadata;


nit: I guess the other option would be something like a handlesReadRepair() method that only ReadRepairVerbHandler overrides, but it's literally called ReadRepairVerbHandler, and we probably won't have something else handle RR mutations.

In any case, I'm remembering blocking RR is going to be reworked for migration anyway, so ignore me :D

maedhroz · 2026-04-02T16:52:22Z

src/java/org/apache/cassandra/service/replication/migration/KeyspaceMigrationInfo.java

+                                              @Nonnull Collection<String> columnFamilies)
+    {
+        Iterable<TableMetadata> tables;
+        if (!columnFamilies.isEmpty())


nit: This is almost a case where null would be nice to indicate "all tables", in the sense that an empty collection might be more likely than null to indicate incorrect argument construction.

That is an artifact of how RepairOption treats the empty set as "all tables". I'll update this method to use null and then fix it at the caller to convert an empty set to null.

maedhroz · 2026-04-02T17:17:46Z

src/java/org/apache/cassandra/repair/RepairCoordinator.java

+            RepairTask task = new PreviewRepairTask(this, state.id, neighborsAndRanges.filterCommonRanges(state.keyspace, cfnames), neighborsAndRanges.shouldExcludeDeadParticipants, cfnames);
+            return task.perform(executor, validationScheduler)
+                       .<Pair<CoordinatedRepairResult, Supplier<String>>>map(r -> Pair.create(r, task::successMessage))
+                       .addCallback((s, f) -> executor.shutdown());


This block here is duplicated 3 more times below. The original code here avoided that by returning after the if/else stuff, but we could just delegate to a submitRepairTask() or something similar.

Added a helper.

maedhroz · 2026-04-02T17:26:57Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+                RepairJobDesc desc = new RepairJobDesc(parentSession, TimeUUID.Generator.nextTimeUUID(),
+                                                       keyspace, "Mutation Tracking Sync", List.of(range));
+                MutationTrackingSyncCoordinator syncCoordinator = new MutationTrackingSyncCoordinator(
+                    coordinator.ctx, desc, commonRange.endpoints, metadata);


MutationTrackingSyncCoordinator syncCoordinator = new MutationTrackingSyncCoordinator(coordinator.ctx, desc, commonRange.endpoints, metadata);

...might be a little easier on the eyes.

maedhroz · 2026-04-02T17:34:56Z

src/java/org/apache/cassandra/repair/RepairCoordinator.java

+                            Pair<CoordinatedRepairResult, Supplier<String>> irPair = Pair.create(irResult, incrementalTask::successMessage);
+                            mtTask.perform(executor, validationScheduler)
+                                    .addCallback(
+                                            mtResult -> result.trySuccess(irPair),


Do we need to handle partial failure here? (i.e. Do we just return the irPair result if the MT task partially fails?)

A failure at any step is a failure of the entire thing since we didn't complete the entire repair. That is what this should be doing which is return failure immediately once any step fails.

maedhroz · 2026-04-02T17:36:57Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+     * Determines if this keyspace should use mutation tracking incremental repair.
+     * Returns true if:
+     * - Keyspace uses mutation tracking replication, OR
+     * - Keyspace is currently migrating (either direction)


nit: Not strictly true if migrating to untracked?

I'll address that in the follow up where I am changing migration from tracked to untracked to be instant.

maedhroz · 2026-04-02T17:38:46Z

src/java/org/apache/cassandra/repair/MutationTrackingIncrementalRepairTask.java

+            for (Range<Token> range : commonRange.ranges)
+            {
+                RepairJobDesc desc = new RepairJobDesc(parentSession, TimeUUID.Generator.nextTimeUUID(),
+                                                       keyspace, "Mutation Tracking Sync", List.of(range));


Table name is meaningless here, right?

Yes but I figured for debugging purposes it's clearer to not leave it empty.

maedhroz · 2026-04-02T19:04:27Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+
+        if (overlappingShards.isEmpty())
+        {
+            completionFuture.setSuccess(null);


nit: Might be nice to have a DEBUG level log message to indicate this happened.

Converted it a checkState

maedhroz · 2026-04-02T19:07:01Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+            }
+            // Always include the local node
+            liveHostIds.add(metadata.directory.peerId(ctx.broadcastAddressAndPort()).id());
+        }


nit: If we just build the liveHostIds at construction time, could we make it final?

Yes I'll make it a final ImmutableSet

maedhroz · 2026-04-02T19:19:23Z

src/java/org/apache/cassandra/replication/MutationTrackingSyncCoordinator.java

+        if (completionFuture.isDone())
+            return;
+
+        recaptureTargets();


It looks like this is called from updateReplicatedOffsets(), but does that mean we keep expanding the targets after the initial round of sync requests? (i.e. If there are ongoing writes, can this cause the whole IR to time out?)

Yeah this shouldn't occur on offsets received. I missed this coming in from the original PR.

…o checkState since it shouldn't actually happen

…ackingIncrementalRepairTask and improve test coverage

aparnanaik0522 and others added 30 commits March 18, 2026 16:23

CEP-45: Incremental Repair Blocking Wait for offsets

a249e80

end--no-edit

9e750e4

Create new test to validate inc repair on ALL replicas

5f51ed1

SyncCoordinatorTest file fix

4aced30

Change shardStates from CHM -> HM

42b465e

Fix possible shard staleness

30cf06f

Fix for happens-before

92ab440

Fix MutationTrackingIncrementalRepairTask file

6d68ef0

Fix IR still doing anti-compaction and TCM consulted multiple times n…

3ab3b55

…on-atomically

Collect offsets via message exchange

f0f68c3

Remove extra timeouts, make top level timeout configurable hot prop

44a3bd3

Fix timeout calculation and clean up error handling

3c457fc

Mutation tracking sync timeout doesn't need to be that long. If it di…

17095cd

…dn't work in 2 minutes it won't work.

Using IncrementalRepairTask directly instead of embedding inside Muta…

65996e4

…tionTrackingIncrementalRepairTask

Add support for force and with hosts

f99c9d5

Fix MutationTrackingSyncCoordinatorTest

4774648

Clean up/fix result handling when pairing incremental repair with MT …

d591087

…repair

Fix java.lang.IllegalStateException: Attempted to create a new keyspa…

42e23c2

…ce shard for keyspace distributed_test_keyspace, but it already exists on bounce

Add MutationTrackingRepairTest

65a366e

checkpoint before big mess

886e4bd

During migration incremental repair might legitimately need to add ss…

2e27603

…tables

During migration SSTableWriter.finalizeMetadata() needs to accept IR …

d42314e

…marking sstables repaired to effect the migration

Can't run MT sync during migration away from mutation tracking becaus…

94cf54b

…e it will hang, so just use IR instead

RepairCoordinator add logging

9cf77b9

During mutation tracking migration restrict repairs to being either e…

adf3638

…ntirely inside migrated ranges or entirely outside, but not both

Spurious error trying to set failure on promise when it has already b…

b4b06b3

…een completed, use tryFailure instead

Allow read repair mutations to be applied during migration

3f6f640

Finish MutationTrackingRepairTest

20c464c

Final test and bug fixes after rebase

04c49ba

A bunch of self review changes/improvements

15a98da

maedhroz reviewed Apr 1, 2026

View reviewed changes

maedhroz reviewed Apr 2, 2026

View reviewed changes

aweisberg added 10 commits April 2, 2026 16:06

Add MutationTrackingMigrationState.isMigrating

c787044

SSTableWriter nit to simplify

3331b66

MutationTrackingSyncRequest javadoc formatting

0d18d4b

MutationTrackingIncrementalRepairTask Convert empty ranges handling t…

2ebab7e

…o checkState since it shouldn't actually happen

Clean up error handling in MutationTrackingSyncCoordinator/MutationTr…

294c024

…ackingIncrementalRepairTask and improve test coverage

Review feedback reduce duplication

aa78ecf

Most of MutationTrackingSyncCoordinatorFeedback

46ab151

Add test case for topology changes causing failure

04d75e7

More feedback/cleanup for MutationSyncCoordinator

069a449

Nit assertRangesNotMixedMigration handling of empty columnFamilies

0739dce

Conversation

aweisberg commented Mar 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aweisberg Apr 6, 2026 •

edited

Loading

aweisberg Apr 2, 2026 •

edited

Loading