sarankk commented on code in PR #238:
URL:
https://github.com/apache/cassandra-analytics/pull/238#discussion_r4017680293
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/ReplicationFactor.java:
##########
@@ -50,6 +51,11 @@
* "replication_factor" : 1
* }
* }
+ * <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
Review 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?
```suggestion
* replicas of which one is transient. Witness replicas (CEP-46) reuse this
form, so
```
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/ReplicationFactor.java:
##########
@@ -108,11 +114,44 @@ public static ReplicationFactor simpleStrategy(int rf)
private final ReplicationStrategy replicationStrategy;
@NotNull
private final Map<String, Integer> options;
+ /**
+ * Per-datacenter count of transient (witness) replicas, parsed from the
{@code <replicas>/<transient>} form.
+ * A datacenter absent from this map has no transient replicas. Always
empty for untracked keyspaces using the
+ * plain {@code <replicas>} form, which keeps behaviour identical for
those keyspaces.
+ */
+ @NotNull
+ private final Map<String, Integer> transientOptions;
+ /**
+ * Lenient parse: a replication value that cannot be parsed is logged and
its datacenter omitted. Retained for
+ * callers that tolerate a partial replication factor.
+ *
+ * @param options the raw replication map, including the {@code class}
entry
+ */
public ReplicationFactor(@NotNull Map<String, String> options)
+ {
+ this(options, false);
+ }
+
+ /**
+ * Strict parse: a replication value that cannot be parsed raises {@link
IllegalArgumentException} naming the
+ * offending datacenter, rather than silently omitting it. Prefer this
when a partial replication factor would
+ * produce a misleading failure later.
+ *
+ * @param options the raw replication map, including the {@code class}
entry
+ * @return the parsed replication factor
+ * @throws IllegalArgumentException when any replication value cannot be
parsed
+ */
+ public static ReplicationFactor parseStrict(@NotNull Map<String, String>
options)
+ {
+ return new ReplicationFactor(options, true);
+ }
+
+ private ReplicationFactor(@NotNull Map<String, String> options, boolean
strict)
{
this.replicationStrategy =
ReplicationFactor.ReplicationStrategy.getEnum(options.get("class"));
this.options = new LinkedHashMap<>(options.size());
Review Comment:
If we plan to merge the maps, shall we make this immutable ?
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/ReplicationFactor.java:
##########
@@ -108,11 +114,44 @@ public static ReplicationFactor simpleStrategy(int rf)
private final ReplicationStrategy replicationStrategy;
@NotNull
private final Map<String, Integer> options;
+ /**
+ * Per-datacenter count of transient (witness) replicas, parsed from the
{@code <replicas>/<transient>} form.
+ * A datacenter absent from this map has no transient replicas. Always
empty for untracked keyspaces using the
+ * plain {@code <replicas>} form, which keeps behaviour identical for
those keyspaces.
+ */
+ @NotNull
+ private final Map<String, Integer> transientOptions;
Review 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`?
##########
cassandra-analytics-common/src/test/java/org/apache/cassandra/spark/data/ReplicationFactorTests.java:
##########
@@ -111,4 +118,296 @@ public void testEquality()
assertThat(replicationFactor1).isEqualTo(replicationFactor2);
assertThat(replicationFactor1.hashCode()).isEqualTo(replicationFactor2.hashCode());
}
+
+ // Transient / witness replicas: the <replicas>/<transient> form, reused
by witness replicas under
+ // mutation tracking (CEP-45/CEP-46)
+
+ @Test
+ public void testNoTransientReplicasByDefault()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3"));
+ assertThat(replicationFactor.hasTransientReplicas()).isFalse();
+ assertThat(replicationFactor.getTransientOptions()).isEmpty();
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(3);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(0);
+
assertThat(replicationFactor.getFullReplicas("datacenter1")).isEqualTo(3);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(0);
+ }
+
+ @Test
+ public void testTransientReplicasSingleDatacenter()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1"));
+ assertThat(replicationFactor.hasTransientReplicas()).isTrue();
+ // total keeps its original meaning: all replicas, witnesses included
+
assertThat(replicationFactor.getOptions().get("datacenter1")).isEqualTo(Integer.valueOf(3));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(1);
+
assertThat(replicationFactor.getFullReplicas("datacenter1")).isEqualTo(2);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(1);
+ }
+
+ @Test
+ public void testTransientReplicasMultipleDatacenters()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1",
+ "datacenter2", "3/1"));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(6);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(4);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testMixedTransientAndFullDatacenters()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1",
+ "datacenter2", "3"));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(6);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(5);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(1);
+
assertThat(replicationFactor.getTransientReplicas("datacenter2")).isEqualTo(0);
+
assertThat(replicationFactor.getFullReplicas("datacenter2")).isEqualTo(3);
+ // datacenter2 has no transient replicas, so it must be absent rather
than mapped to zero
+
assertThat(replicationFactor.getTransientOptions()).containsOnlyKeys("datacenter1");
+ }
+
+ @Test
+ public void testTransientReplicasSimpleStrategy()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "SimpleStrategy",
+ "replication_factor", "3/1"));
+ assertThat(replicationFactor.getReplicationStrategy())
+ .isEqualTo(ReplicationFactor.ReplicationStrategy.SimpleStrategy);
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testTransientReplicasWithWhitespace()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", " 3 / 1 "));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testTransientEqualToTotalIsRejected()
+ {
+ // Cassandra requires at least one full replica, so 3/3 is invalid
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/3"));
+
assertThat(replicationFactor.getOptions()).doesNotContainKey("datacenter1");
Review Comment:
We should throw in this scenario instead, if Cassandra does not allow it,
Sidecar should fail as well.
##########
cassandra-analytics-core/src/test/java/org/apache/cassandra/spark/utils/CqlUtilsTest.java:
##########
@@ -142,6 +142,84 @@ public void testExtractReplicationFactor(CassandraBridge
bridge)
assertThat(systemSchemaRf.getOptions()).isEqualTo(ImmutableMap.of());
}
+ @Test
+ public void testExtractReplicationFactorWithWitnessReplicas()
+ {
+ // 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' "
Review Comment:
`replication_type = 'tracked'` this field is not yet supported in Sidecar
schema response, shall we remove this?
##########
cassandra-analytics-common/src/test/java/org/apache/cassandra/spark/data/ReplicationFactorTests.java:
##########
@@ -111,4 +118,296 @@ public void testEquality()
assertThat(replicationFactor1).isEqualTo(replicationFactor2);
assertThat(replicationFactor1.hashCode()).isEqualTo(replicationFactor2.hashCode());
}
+
+ // Transient / witness replicas: the <replicas>/<transient> form, reused
by witness replicas under
+ // mutation tracking (CEP-45/CEP-46)
+
+ @Test
+ public void testNoTransientReplicasByDefault()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3"));
+ assertThat(replicationFactor.hasTransientReplicas()).isFalse();
+ assertThat(replicationFactor.getTransientOptions()).isEmpty();
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(3);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(0);
+
assertThat(replicationFactor.getFullReplicas("datacenter1")).isEqualTo(3);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(0);
+ }
+
+ @Test
+ public void testTransientReplicasSingleDatacenter()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1"));
+ assertThat(replicationFactor.hasTransientReplicas()).isTrue();
+ // total keeps its original meaning: all replicas, witnesses included
+
assertThat(replicationFactor.getOptions().get("datacenter1")).isEqualTo(Integer.valueOf(3));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(1);
+
assertThat(replicationFactor.getFullReplicas("datacenter1")).isEqualTo(2);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(1);
+ }
+
+ @Test
+ public void testTransientReplicasMultipleDatacenters()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1",
+ "datacenter2", "3/1"));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(6);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(4);
+
assertThat(replicationFactor.getTransientReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testMixedTransientAndFullDatacenters()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1",
+ "datacenter2", "3"));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(6);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(5);
+
assertThat(replicationFactor.getTransientReplicas("datacenter1")).isEqualTo(1);
+
assertThat(replicationFactor.getTransientReplicas("datacenter2")).isEqualTo(0);
+
assertThat(replicationFactor.getFullReplicas("datacenter2")).isEqualTo(3);
+ // datacenter2 has no transient replicas, so it must be absent rather
than mapped to zero
+
assertThat(replicationFactor.getTransientOptions()).containsOnlyKeys("datacenter1");
+ }
+
+ @Test
+ public void testTransientReplicasSimpleStrategy()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "SimpleStrategy",
+ "replication_factor", "3/1"));
+ assertThat(replicationFactor.getReplicationStrategy())
+ .isEqualTo(ReplicationFactor.ReplicationStrategy.SimpleStrategy);
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testTransientReplicasWithWhitespace()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", " 3 / 1 "));
+ assertThat(replicationFactor.getTotalReplicationFactor()).isEqualTo(3);
+ assertThat(replicationFactor.getFullReplicationFactor()).isEqualTo(2);
+ }
+
+ @Test
+ public void testTransientEqualToTotalIsRejected()
+ {
+ // Cassandra requires at least one full replica, so 3/3 is invalid
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/3"));
+
assertThat(replicationFactor.getOptions()).doesNotContainKey("datacenter1");
+ }
+
+ @Test
+ public void testMalformedTransientValuesAreSkipped()
+ {
+ for (String malformed : new String[]{ "3/", "/1", "3/1/1", "3/x",
"x/1", "3/-1", "" })
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", malformed));
+ assertThat(replicationFactor.getOptions())
+ .as("malformed value '%s' should not produce a replication factor
entry", malformed)
+ .doesNotContainKey("datacenter1");
+ }
+ }
+
+ @Test
+ public void testGetFullReplicasUnknownDatacenter()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1"));
+ assertThatThrownBy(() -> replicationFactor.getFullReplicas("nosuchdc"))
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ public void testTransientOptionsForUnknownDatacenterRejected()
+ {
+ assertThatThrownBy(() -> new ReplicationFactor(
+ ReplicationFactor.ReplicationStrategy.NetworkTopologyStrategy,
+ ImmutableMap.of("datacenter1", 3),
+ ImmutableMap.of("datacenter2", 1)))
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ public void testEqualityConsidersTransientReplicas()
+ {
+ ReplicationFactor full = new ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3"));
+ ReplicationFactor withTransient = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "3/1"));
+ assertThat(full).isNotEqualTo(withTransient);
+ assertThat(full.hashCode()).isNotEqualTo(withTransient.hashCode());
+ }
+
+ @Test
+ public void testNegativeReplicationFactorIsRejected()
+ {
+ ReplicationFactor replicationFactor = new
ReplicationFactor(ImmutableMap.of(
+ "class", "NetworkTopologyStrategy",
+ "datacenter1", "-3"));
+
assertThat(replicationFactor.getOptions()).doesNotContainKey("datacenter1");
Review Comment:
Should we throw in this scenario as well?
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/ReplicationFactor.java:
##########
@@ -108,11 +114,44 @@ public static ReplicationFactor simpleStrategy(int rf)
private final ReplicationStrategy replicationStrategy;
@NotNull
private final Map<String, Integer> options;
+ /**
+ * Per-datacenter count of transient (witness) replicas, parsed from the
{@code <replicas>/<transient>} form.
+ * A datacenter absent from this map has no transient replicas. Always
empty for untracked keyspaces using the
+ * plain {@code <replicas>} form, which keeps behaviour identical for
those keyspaces.
+ */
+ @NotNull
+ private final Map<String, Integer> transientOptions;
+ /**
+ * Lenient parse: a replication value that cannot be parsed is logged and
its datacenter omitted. Retained for
Review Comment:
In what scenario do we anticipate using lenient parse?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
