You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "aweisberg (via GitHub)" <gi...@apache.org> on 2025/09/15 19:05:17 UTC
[PR] CEP-46 witnesses [cassandra]
aweisberg opened a new pull request, #4378:
URL: https://github.com/apache/cassandra/pull/4378
This is some initial cleanup removing a lot of references to transient replication from a lot of the sstable plumbing. I didn't remove the field from the format metadata yet although we should do that as part of the next format change.
It also has the few tweaks needed to get single partition reads and writes working.
Known bugs:
Needs fixing now
* ReplicaPlans.contactForRead doesn’t ensure any full replicas will be read from at ONE
* Fix witness newly introduced bug where repair doesn’t correctly terminate on transient nodes and mark their sstables as repaired preventing nodetool cleanup from cleaning transient data
Could be fixed later
* Fix bug where partition range read command can send data requests to transient replicas because the Replica doesn’t match what is actually being queried by the read command
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383301731
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateTableStatement.java:
##########
@@ -167,11 +166,12 @@ public Keyspaces apply(ClusterMetadata metadata)
TableMetadata table = builder.build();
table.validate();
- if (keyspace.replicationStrategy.hasTransientReplicas()
- && table.params.readRepair != ReadRepairStrategy.NONE)
- {
- throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
- }
+ // TODO (review): This can be removed right? ReadRepair is effectively not done anymore so the setting doesn't matter
+// if (keyspace.replicationStrategy.hasTransientReplicas()
+// && table.params.readRepair != ReadRepairStrategy.NONE)
+// {
+// throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
+// }
Review Comment:
I'm going to punt on this for now. Requiring things makes it blow up on existing schema when maybe it doesn't need to? Makes it harder to enable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#issuecomment-3340066836
I have a separate PR for the rename https://github.com/apache/cassandra/pull/4174 I just haven't had a chance to really push that through and I was also wondering if we should merge the rename back to 4.0 to ease merge pain in the long run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383289713
##########
src/java/org/apache/cassandra/locator/ReplicaPlans.java:
##########
@@ -632,6 +632,9 @@ E select(ConsistencyLevel consistencyLevel, L liveAndDown, L live)
}
};
+ // TODO (review): Cheap quorums are not a goal for now, would really require speculation for writes
+ // and would trigger reconciliation work later, potentially quite a lot so I don't think it makes sense.
+ // So just remove selector?
Review Comment:
I'm going to leave this in for now to avoid disruption.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383305667
##########
src/java/org/apache/cassandra/db/AbstractReadCommandVerbHandler.java:
##########
@@ -107,6 +108,7 @@ else if (localComparisonEpoch.isAfter(readCommand.serializedAtEpoch()))
private ClusterMetadata checkTokenOwnership(ClusterMetadata metadata, Message<T> message)
{
+ boolean acceptsTransient = message.verb() != Verb.READ_REQ;
Review Comment:
Actually the summary requests go through here so I will use summary request verb to check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383252658
##########
src/java/org/apache/cassandra/db/AbstractReadCommandVerbHandler.java:
##########
@@ -107,6 +108,7 @@ else if (localComparisonEpoch.isAfter(readCommand.serializedAtEpoch()))
private ClusterMetadata checkTokenOwnership(ClusterMetadata metadata, Message<T> message)
{
+ boolean acceptsTransient = message.verb() != Verb.READ_REQ;
Review Comment:
Nothing reads from transient replicas via this path and verb handler now since Blake reworked summary requests to be their own verb. I'll update this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383284998
##########
src/java/org/apache/cassandra/tools/SSTableMetadataViewer.java:
##########
@@ -422,8 +422,9 @@ private void printSStableMetadata(File file, boolean scan) throws IOException
field("ClusteringTypes", clusteringTypes.toString());
field("StaticColumns", FBUtilities.toString(statics));
field("RegularColumns", FBUtilities.toString(regulars));
- if (stats != null)
- field("IsTransient", stats.isTransient);
+ // TODO (review): This should be removed right? Nothing depends on it?
+// if (stats != null)
+// field("IsTransient", stats.isTransient);
Review Comment:
The problem is poorly written tools can see the field missing and start erroring out. I'll put it back in for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2364903990
##########
src/java/org/apache/cassandra/db/AbstractReadCommandVerbHandler.java:
##########
@@ -107,6 +108,7 @@ else if (localComparisonEpoch.isAfter(readCommand.serializedAtEpoch()))
private ClusterMetadata checkTokenOwnership(ClusterMetadata metadata, Message<T> message)
{
+ boolean acceptsTransient = message.verb() != Verb.READ_REQ;
Review Comment:
Confirming this will include RANGE_REQ later on
##########
src/java/org/apache/cassandra/db/compaction/CompactionStrategyManager.java:
##########
@@ -1388,7 +1371,7 @@ public CleanupSummary releaseRepairData(Collection<TimeUUID> sessions)
readLock.lock();
try
{
- for (PendingRepairManager prm : Iterables.concat(pendingRepairs.getManagers(), transientRepairs.getManagers()))
+ for (PendingRepairManager prm : Iterables.concat(pendingRepairs.getManagers()))
Review Comment:
No need to concat anymore
##########
src/java/org/apache/cassandra/tools/SSTableMetadataViewer.java:
##########
@@ -422,8 +422,9 @@ private void printSStableMetadata(File file, boolean scan) throws IOException
field("ClusteringTypes", clusteringTypes.toString());
field("StaticColumns", FBUtilities.toString(statics));
field("RegularColumns", FBUtilities.toString(regulars));
- if (stats != null)
- field("IsTransient", stats.isTransient);
+ // TODO (review): This should be removed right? Nothing depends on it?
+// if (stats != null)
+// field("IsTransient", stats.isTransient);
Review Comment:
I support removal 👍
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -1430,7 +1393,7 @@ private void serializeHeader(ReadCommand command, DataOutputPlus out, int versio
out.writeByte(
digestFlag(command.isDigestQuery())
| indexFlag(null != command.indexQueryPlan())
- | acceptsTransientFlag(command.acceptsTransient())
+ // | acceptsTransientFlag(false) Deprecated flag, could be reused?
| needsReconciliationFlag(command.rowFilter().needsReconciliation())
Review Comment:
`needsReconciliationFlag` was added for non-strict filtering, should probably rename for clarity now that read reconciliation is its own concept
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateTableStatement.java:
##########
@@ -167,11 +166,12 @@ public Keyspaces apply(ClusterMetadata metadata)
TableMetadata table = builder.build();
table.validate();
- if (keyspace.replicationStrategy.hasTransientReplicas()
- && table.params.readRepair != ReadRepairStrategy.NONE)
- {
- throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
- }
+ // TODO (review): This can be removed right? ReadRepair is effectively not done anymore so the setting doesn't matter
+// if (keyspace.replicationStrategy.hasTransientReplicas()
+// && table.params.readRepair != ReadRepairStrategy.NONE)
+// {
+// throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
+// }
Review Comment:
I think we should require BLOCKING for tracked keyspaces for now. Users could previously switch to NONE if they preferred write atomicity over read monotonicity and wanted slightly better performance, but with mutation tracking there wouldn't be a semantics tradeoff, and the performance of read reconciliation should be even better than read repair anyway.
The reason I think we should defer support for NONE is that we may decide to do an equivalent of full repair with range reads, and there isn't a strong justification to support both modes yet.
##########
src/java/org/apache/cassandra/db/Keyspace.java:
##########
@@ -629,8 +634,42 @@ private Future<?> applyInternalTracked(Mutation mutation, Promise<?> future)
continue;
}
+ // If this range is only witnessed then don't apply the update to the underlying column family store
+ // We still want the mutation tracking log to see the update so that it can witness it
+ // and participate in reconciliation of the mutation
+ AbstractReplicationStrategy replicationStrategy = cfs.keyspace.getReplicationStrategy();
+ if (replicationStrategy.hasTransientReplicas())
+ {
+ RangesAtEndpoint localRanges = replicationStrategy.getLocalRanges(cm);
+ Token token = upd.partitionKey().getToken();
+ boolean foundMatch = false;
+ // TODO (review): Races and this being an out of range mutation, it's silently accepting the mutation to the log
+ // but not applying to the CF. This was already checked, but in a race what happens? Double check ack handling
+ // and whether the voting group is checked to match safely
+ for (Range<Token> r : localRanges.onlyFull().ranges())
Review Comment:
If we're losing the range (coordinator is behind and believes replica owns the range), we may write something to an SSTable that's not read, and will be purged on next cleanup.
If we're gaining the range (coordinator is ahead and believes replica owns the range), TCM should try to catch up before we process the message and get here.
##########
src/java/org/apache/cassandra/locator/ReplicaPlans.java:
##########
@@ -632,6 +632,9 @@ E select(ConsistencyLevel consistencyLevel, L liveAndDown, L live)
}
};
+ // TODO (review): Cheap quorums are not a goal for now, would really require speculation for writes
+ // and would trigger reconciliation work later, potentially quite a lot so I don't think it makes sense.
+ // So just remove selector?
Review Comment:
I support removing support for cheap quorums. I _could_ imagine a world where we deprioritize delivery of new writes to witnesses if reconciliation is behind, but even a change like that would require more justification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aweisberg (via GitHub)" <gi...@apache.org>.
aweisberg commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383240718
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -1430,7 +1393,7 @@ private void serializeHeader(ReadCommand command, DataOutputPlus out, int versio
out.writeByte(
digestFlag(command.isDigestQuery())
| indexFlag(null != command.indexQueryPlan())
- | acceptsTransientFlag(command.acceptsTransient())
+ // | acceptsTransientFlag(false) Deprecated flag, could be reused?
| needsReconciliationFlag(command.rowFilter().needsReconciliation())
Review Comment:
Is this in scope for this change set?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383374536
##########
src/java/org/apache/cassandra/cql3/statements/schema/CreateTableStatement.java:
##########
@@ -167,11 +166,12 @@ public Keyspaces apply(ClusterMetadata metadata)
TableMetadata table = builder.build();
table.validate();
- if (keyspace.replicationStrategy.hasTransientReplicas()
- && table.params.readRepair != ReadRepairStrategy.NONE)
- {
- throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
- }
+ // TODO (review): This can be removed right? ReadRepair is effectively not done anymore so the setting doesn't matter
+// if (keyspace.replicationStrategy.hasTransientReplicas()
+// && table.params.readRepair != ReadRepairStrategy.NONE)
+// {
+// throw ire("read_repair must be set to 'NONE' for transiently replicated keyspaces");
+// }
Review Comment:
Makes sense, I support moving forward with that option essentially ignored for now. We could also drop a client warning if a user has mutation tracking enabled for a keyspace with read repair NONE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CEP-46 witnesses [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #4378:
URL: https://github.com/apache/cassandra/pull/4378#discussion_r2383245223
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -1430,7 +1393,7 @@ private void serializeHeader(ReadCommand command, DataOutputPlus out, int versio
out.writeByte(
digestFlag(command.isDigestQuery())
| indexFlag(null != command.indexQueryPlan())
- | acceptsTransientFlag(command.acceptsTransient())
+ // | acceptsTransientFlag(false) Deprecated flag, could be reused?
| needsReconciliationFlag(command.rowFilter().needsReconciliation())
Review Comment:
No, just a thought while I was reviewing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org