CASSANALYTICS-194: Parse <replicas>/<transient> replication factor fo… - #238
mansikhara wants to merge 3 commits into
Conversation
…r witness-enabled keyspaces Cassandra accepts a replication factor of the form <replicas>/<transient>, which witness replicas under mutation tracking reuse, so a witness-enabled keyspace declares 'datacenter1': '3/1' meaning three replicas of which one is a witness. CqlUtils.extractReplicationFactor() passed each datacenter value to Integer.parseInt, so a bulk read against any such keyspace failed during job setup with an uncaught NumberFormatException. Transient counts are now tracked per datacenter alongside the existing totals and exposed through getFullReplicationFactor(), getTransientReplicationFactor(), getFullReplicas(dc), getTransientReplicas(dc) and hasTransientReplicas(). getTotalReplicationFactor() keeps its existing meaning of all replicas including witnesses, so behaviour for untracked keyspaces is unchanged. Parsing applies the same constraints Cassandra enforces in locator.ReplicationFactor.validate. A new parseStrict factory reports an unparseable or empty replication map at parse time rather than dropping the datacenter and failing later with a misleading "DC not found in replication factor"; the lenient constructor is retained unchanged for CDC callers. The Kryo serializer and CassandraRing's hand-rolled JDK readObject/writeObject are updated so the new field survives serialization to Spark executors. CassandraRing gains a pinned serialVersionUID and a format-version byte, because the class signature did not change and an older stream would otherwise be silently misread rather than rejected. Prerequisite for CASSANALYTICS-164. patch by Mansi Khara; reviewed by TBD for CASSANALYTICS-194
sarankk
left a comment
There was a problem hiding this comment.
Thanks Mansi, left few comments
| * } | ||
| * <p> | ||
| * Replica counts may also use the {@code <replicas>/<transient>} form, e.g. {@code "DC1" : "3/1"}, meaning three | ||
| * replicas of which one is transient. Witness replicas under mutation tracking (CEP-45/CEP-46) reuse this form, so |
There was a problem hiding this comment.
Nit: Transient replication was added before MT, it was updated to work on top of MT later. Also why do we use term reuse here?
| * replicas of which one is transient. Witness replicas under mutation tracking (CEP-45/CEP-46) reuse this form, so | |
| * replicas of which one is transient. Witness replicas (CEP-46) reuse this form, so |
There was a problem hiding this comment.
Good catch, you are right that transient replication came first. Applied your suggestion, thanks!
| private final Map<String, Integer> transientOptions; | ||
|
|
||
| /** | ||
| * Lenient parse: a replication value that cannot be parsed is logged and its datacenter omitted. Retained for |
There was a problem hiding this comment.
In what scenario do we anticipate using lenient parse?
There was a problem hiding this comment.
None, as it turns out. I checked the callers and there are only two, ReplicationFactorSupplier and CdcOptions, and both build hardcoded maps that cannot fail to parse. So I have removed the lenient path entirely and made strict the only behaviour! Thanks for the find
| * plain {@code <replicas>} form, which keeps behaviour identical for those keyspaces. | ||
| */ | ||
| @NotNull | ||
| private final Map<String, Integer> transientOptions; |
There was a problem hiding this comment.
Options hold dc : 3 and transient options hold dc : 1 for transient replication dc: 3/1. The maps could diverge. Shall we combine the 2 maps into 1 using ReplicaCounts?
There was a problem hiding this comment.
Agreed and done, thanks!
| private ReplicationFactor(@NotNull Map<String, String> options, boolean strict) | ||
| { | ||
| this.replicationStrategy = ReplicationFactor.ReplicationStrategy.getEnum(options.get("class")); | ||
| this.options = new LinkedHashMap<>(options.size()); |
There was a problem hiding this comment.
If we plan to merge the maps, shall we make this immutable ?
| ReplicationFactor replicationFactor = new ReplicationFactor(ImmutableMap.of( | ||
| "class", "NetworkTopologyStrategy", | ||
| "datacenter1", "3/3")); | ||
| assertThat(replicationFactor.getOptions()).doesNotContainKey("datacenter1"); |
There was a problem hiding this comment.
We should throw in this scenario instead, if Cassandra does not allow it, Sidecar should fail as well.
There was a problem hiding this comment.
Great suggestions! Done.
| ReplicationFactor replicationFactor = new ReplicationFactor(ImmutableMap.of( | ||
| "class", "NetworkTopologyStrategy", | ||
| "datacenter1", "-3")); | ||
| assertThat(replicationFactor.getOptions()).doesNotContainKey("datacenter1"); |
There was a problem hiding this comment.
Should we throw in this scenario as well?
| // Witness-enabled keyspace as created by Cassandra's WitnessAlwaysReadsFullReplicaTest on the | ||
| // cep-45-mutation-tracking branch: the <replicas>/<transient> form plus replication_type = 'tracked' | ||
| String schema = "CREATE KEYSPACE witnessks WITH REPLICATION = {'class': 'NetworkTopologyStrategy', " | ||
| + "'datacenter1': '3/1', 'datacenter2': '3/1'} AND replication_type = 'tracked' " |
There was a problem hiding this comment.
replication_type = 'tracked' this field is not yet supported in Sidecar schema response, shall we remove this from example?
There was a problem hiding this comment.
You are right, I confirmed it. Removed it from these fixtures since it is irrelevant to replication factor parsing anyway.
…ictly
Combines the per-datacenter total and transient counts into a single Map<String, ReplicaCounts>
instead of two parallel maps. The maps could previously drift apart, since nothing tied
options={dc1=3} to transientOptions={dc1=1}; holding them together makes that unrepresentable.
ReplicaCounts becomes a public nested type exposing allReplicas, fullReplicas and
transientReplicas. getOptions() and getTransientOptions() are retained, now derived and
unmodifiable, so CassandraRing, ConsistencyLevel and PartitionedDataLayer are unaffected.
Removes the lenient parse. Its only two callers built hardcoded maps that cannot fail to parse,
so it was never needed, and silently dropping a datacenter that Cassandra itself would reject is
the wrong default: 3/3 and a negative replication factor now raise rather than being skipped.
parseStrict therefore disappears as a separate factory, since strict is the only behaviour.
Corrects the javadoc: transient replication predates mutation tracking, and it is witness
replicas (CEP-46) that reuse the form. Drops replication_type from the replication factor test
fixtures, since Sidecar builds its schema response from the driver's exportAsString() which does
not emit that property, and it is irrelevant to replication factor parsing.
e2d75d0 to
3a07399
Compare
…r witness-enabled keyspaces
Cassandra accepts a replication factor of the form /, which witness replicas under mutation tracking reuse, so a witness-enabled keyspace declares 'datacenter1': '3/1' meaning three replicas of which one is a witness.
CqlUtils.extractReplicationFactor() passed each datacenter value to Integer.parseInt, so a bulk read against any such keyspace failed during job setup with an uncaught NumberFormatException.
Transient counts are now tracked per datacenter alongside the existing totals and exposed through getFullReplicationFactor(), getTransientReplicationFactor(), getFullReplicas(dc), getTransientReplicas(dc) and hasTransientReplicas(). getTotalReplicationFactor() keeps its existing meaning of all replicas including witnesses, so behaviour for untracked keyspaces is unchanged. Parsing applies the same constraints Cassandra enforces in locator.ReplicationFactor.validate. A new parseStrict factory reports an unparseable or empty replication map at parse time rather than dropping the datacenter and failing later with a misleading "DC not found in replication factor"; the lenient constructor is retained unchanged for CDC callers. The Kryo serializer and CassandraRing's hand-rolled JDK readObject/writeObject are updated so the new field survives serialization to Spark executors.
Prerequisite for CASSANALYTICS-164.