CASSANDRA-21674 CEP-46: update witness replication validation restrictions by bdeggleston · Pull Request #5155 · apache/cassandra

bdeggleston · 2026-09-10T22:39:08Z

Thanks for sending a pull request! Here are some tips if you're new here:

Ensure you have added or run the appropriate tests for your PR.
Be sure to keep the PR description updated to reflect all changes.
Write your PR title to summarize what this PR proposes.
If possible, provide a concise example to reproduce the issue for a faster review.
Read our contributor guidelines
If you're making a documentation change, see our guide to documentation contribution

Commit messages should follow the following format:

<One sentence description, usually Jira title or CHANGES.txt summary>

<Optional lengthier description (context on patch)>

patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####

Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>

The Cassandra Jira

Witness replicas require mutation tracking, and tracked reads never build a repairing ReadRepair, so the read_repair table option cannot affect a read that reaches a witness. CASSANDRA-20930 disabled this check in CreateTableStatement but left the AlterTableStatement and CopyTableStatement copies in place.

Reads for a range still pending migration take the untracked path, which refuses transient replicas, and those ranges rely on blocking read repair that a witness cannot serve. Two conditions are needed because AlterSchema starts the migration after the statement validates, so a statement that both enables tracking and adds witnesses cannot be seen in the migration state.

A promoted witness holds no data for the range it witnessed and quorum reads would count it, so promotion needs cassandra.allow_unsafe_witness_promotion, which is settable over JMX because QA performs it deliberately. The comparison is per datacenter, since NetworkTopologyStrategy sums its per-datacenter factors and aggregates hide a promotion offset by a reduction elsewhere. Dropping a witness stays legal: replica ordering removes it from the replica set rather than promoting it, and the remaining full replicas already hold the data.

Witnesses never serve data reads, since TrackedRead picks a full replica and summaries are built from the mutation tracking log, so an index on a witness is never consulted. The restriction was also asymmetric: CREATE INDEX on an existing witness keyspace was already accepted. Also drops a constant in CreateIndexStatement that was declared and referenced nowhere.

A counter leader resolves the increment against its local data, which a witness does not have for the range it witnesses, so it would write a value discarding every prior increment. findCounterLeaderReplica now considers only full replicas, and TrackedWriteRequest forwards rather than leading when the local replica is a witness.

A tracked prepare sends its data request to one participant and summary requests to the rest, so a witness votes without being asked for data it does not have. Extracts the selection into PaxosPrepare#selectDataNode so the test drives production code rather than a copy of it. No behaviour change.

The list of unsupported features was inherited from 4.0 and most of it no longer holds: lightweight transactions, secondary indexes and counters all work. Records what is actually enforced, why materialized views remain rejected, and the sequences for adopting and removing witnesses.

frankgh

Added some comments to the patch.

frankgh · 2026-09-16T18:13:09Z

+        // A migrating keyspace has pending ranges, and reads for a pending range take the untracked
+        // path, see MigrationRouter#shouldUseTrackedForReads. Those reads would contact a transient
+        // replica, which RangeCommandIterator#executeNormal rejects outright. Migrating a pending range
+        // also relies on blocking read repair to converge the replicas, and a witness cannot take part.


I don't think the comments here add too much value. I would remove them

frankgh · 2026-09-16T18:13:37Z

+        // AlterSchema#maybeUpdateMutationTrackingMigrationState starts the migration after this
+        // statement validates, so the check above cannot see one this statement is about to start.


frankgh · 2026-09-16T18:18:12Z

        return PaxosRepair.getSkipPaxosRepairCompatibilityCheck();
    }

+    public void setAllowUnsafeWitnessPromotion(boolean allow)


NIT:

Suggested change

public void setAllowUnsafeWitnessPromotion(boolean allow)

@Override

public void setAllowUnsafeWitnessPromotion(boolean allow)

frankgh · 2026-09-16T18:18:22Z

+        logger.info("AllowUnsafeWitnessPromotion set to {} via jmx", allow);
+    }
+
+    public boolean getAllowUnsafeWitnessPromotion()


NIT:

Suggested change

public boolean getAllowUnsafeWitnessPromotion()

@Override

public boolean getAllowUnsafeWitnessPromotion()

frankgh · 2026-09-16T18:36:00Z

+                                                        ALLOW_UNSAFE_TRANSIENT_CHANGES.getKey()));
+    }
+
+    private static volatile boolean allowUnsafeWitnessPromotion = ALLOW_UNSAFE_WITNESS_PROMOTION.getBoolean();


I'm a little concerned about this flag. For this to work correctly, we'll need to uniformly set it on all the nodes of the cluster before running the ALTER command. Otherwise, TCM will fail when following the metadata log and attempting to apply the change. The exception will be thrown if any of the nodes configuration diverges. I guess it can also happen when a node replays the log and the state changes, or if for some reason we set this via JMX, and one of the node restarts. I think in general this approach is a little brittle. So we either need to make sure we document this really well, or we need to find some alternate approach.

yeah it's a little dicey. The problem is that we need some sort of escape hatch to turn off witness replication that doesn't rely on mutation tracking working properly. First of all, we can't correctly promote witnesses without stopping client traffic and running repair at the moment. Once we can though, we still need to get out of bad situations.

yeah, makes absolute sense to have the escape hatch. Just wondering how we should document this to make it visible for operators

pushed a commit that moves config gated validation into the pre-commit validation step

frankgh · 2026-09-16T19:03:38Z

+    static Replica selectDataNode(Participants participants, InetAddressAndPort local)
+    {
+        Replica localReplica = participants.lookup(local);
+        if (localReplica != null && localReplica.isFull())


do we need to check here whether the local replica is not pending? similar to the check below in line 507?

Suggested change

if (localReplica != null && localReplica.isFull())

if (localReplica != null && localReplica.isFull() && !participants.electorate.isPending(localReplica.endpoint()))

I guess this is preserving the behavior we had before in lines 450-453 and lines 457-467

nice catch, added a test for this and fixed it. Also found an array sizing issue and fixed it as well

frankgh · 2026-09-16T21:16:41Z

+        if (allow_unsafe_transient_changes)
+            return;
+
+        boolean addsWitnesses = proposed.replicationStrategy.getReplicationFactor().hasTransientReplicas();


I don't think this variable name is accurate, we never check the current KeyspaceMetadata to make a determination on whether witnesses are being added. Maybe rename the variable to reflect the correct meaning?

alternatively we can add a method that checks on a per DC basis whether we are adding witnesses?

static boolean addsWitnesses(AbstractReplicationStrategy current, AbstractReplicationStrategy proposed) { Map<String, ReplicationFactor> before = replicationFactorsByGroup(current); Map<String, ReplicationFactor> after = replicationFactorsByGroup(proposed); for (String group : Sets.union(before.keySet(), after.keySet())) { if (after.getOrDefault(group, ReplicationFactor.ZERO).transientReplicas() > before.getOrDefault(group, ReplicationFactor.ZERO).transientReplicas()) return true; } return false; }

…eStatement.validate

bdeggleston added 7 commits September 10, 2026 14:51

frankgh reviewed Sep 16, 2026

View reviewed changes

bdeggleston added 4 commits September 17, 2026 10:19

remove comments

d4a09f5

Move node-configuration dependent keyspace validation to AlterKeyspac…

3370d88

…eStatement.validate

review feedback

eaed288

Do not select pending nodes as Paxos data node

ad494fa

		// AlterSchema#maybeUpdateMutationTrackingMigrationState starts the migration after this
		// statement validates, so the check above cannot see one this statement is about to start.

	public void setAllowUnsafeWitnessPromotion(boolean allow)
	@Override
	public void setAllowUnsafeWitnessPromotion(boolean allow)

	public boolean getAllowUnsafeWitnessPromotion()
	@Override
	public boolean getAllowUnsafeWitnessPromotion()

	if (localReplica != null && localReplica.isFull())
	if (localReplica != null && localReplica.isFull() && !participants.electorate.isPending(localReplica.endpoint()))

Conversation

bdeggleston commented Sep 10, 2026

Uh oh!

frankgh left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants