mansikhara commented on code in PR #238:
URL:
https://github.com/apache/cassandra-analytics/pull/238#discussion_r4032256575
##########
cassandra-analytics-common/src/main/java/org/apache/cassandra/spark/data/ReplicationFactor.java:
##########
@@ -106,62 +110,204 @@ public static ReplicationFactor simpleStrategy(int rf)
@NotNull
private final ReplicationStrategy replicationStrategy;
+ /**
+ * Per-datacenter replica counts. Holding the total and the transient
count together, rather than in two
+ * parallel maps, means the two cannot drift apart.
+ */
@NotNull
- private final Map<String, Integer> options;
+ private final Map<String, ReplicaCounts> replication;
+ /**
+ * Parses a raw replication map. A value that cannot be parsed, or that
Cassandra itself would reject, raises
+ * {@link IllegalArgumentException} naming the offending datacenter: a
partial replication factor is not usable
+ * and reporting it here avoids a misleading failure later, such as "DC
not found in replication factor".
+ *
+ * @param options the raw replication map, including the {@code class}
entry
+ * @throws IllegalArgumentException when any replication value cannot be
parsed
+ */
public ReplicationFactor(@NotNull Map<String, String> options)
{
this.replicationStrategy =
ReplicationFactor.ReplicationStrategy.getEnum(options.get("class"));
- this.options = new LinkedHashMap<>(options.size());
+ Map<String, ReplicaCounts> parsed = new
LinkedHashMap<>(options.size());
for (Map.Entry<String, String> entry : options.entrySet())
{
if ("class".equals(entry.getKey()))
{
continue;
}
-
try
{
- this.options.put(entry.getKey(),
Integer.parseInt(entry.getValue()));
+ parsed.put(entry.getKey(),
ReplicaCounts.parse(entry.getValue()));
}
- catch (NumberFormatException exception)
+ catch (IllegalArgumentException exception)
{
- LOGGER.warn("Could not parse replication option: {} = {}",
entry.getKey(), entry.getValue());
+ throw new IllegalArgumentException(String.format("Could not
parse replication option: %s = %s",
+
entry.getKey(), entry.getValue()), exception);
}
}
+ validateNotEmpty(this.replicationStrategy, parsed, options);
+ this.replication = Collections.unmodifiableMap(parsed);
}
public ReplicationFactor(@NotNull ReplicationStrategy replicationStrategy,
@NotNull Map<String, Integer> options)
{
- this.replicationStrategy = replicationStrategy;
- this.options = new LinkedHashMap<>(options.size());
-
- if (!replicationStrategy.equals(ReplicationStrategy.LocalStrategy) &&
options.isEmpty())
- {
- throw new RuntimeException(String.format("Could not find
replication info in schema map: %s.", options));
- }
+ this(replicationStrategy, options, Collections.emptyMap());
+ }
+ public ReplicationFactor(@NotNull ReplicationStrategy replicationStrategy,
+ @NotNull Map<String, Integer> options,
+ @NotNull Map<String, Integer> transientOptions)
+ {
+ this.replicationStrategy = replicationStrategy;
+ Map<String, ReplicaCounts> merged = new
LinkedHashMap<>(options.size());
for (Map.Entry<String, Integer> entry : options.entrySet())
Review Comment:
Done! Good eye, thanks!
--
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]
