Refuse unrecognized replication strategy options patch by dbrosius; reviewed by slebresne for CASSANDRA-4795
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/dc94f373 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/dc94f373 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/dc94f373 Branch: refs/heads/cassandra-1.2 Commit: dc94f3730f50005524edf946772e00bdb7c80327 Parents: 8349fce Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Wed Jan 9 11:17:37 2013 +0100 Committer: Sylvain Lebresne <sylv...@datastax.com> Committed: Wed Jan 9 11:17:37 2013 +0100 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../locator/AbstractReplicationStrategy.java | 13 +++++++++---- .../apache/cassandra/locator/LocalStrategy.java | 2 +- .../cassandra/locator/NetworkTopologyStrategy.java | 1 - .../apache/cassandra/locator/SimpleStrategy.java | 9 ++++----- .../apache/cassandra/thrift/CassandraServer.java | 8 ++++++++ 6 files changed, 23 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 733c3a3..3567362 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -24,6 +24,7 @@ * Ensure CL guarantees on digest mismatch (CASSANDRA-5113) * Validate correctly selects on composite partition key (CASSANDRA-5122) * Fix exception when adding collection (CASSANDRA-5117) + * Refuse unrecognized replication strategy options (CASSANDRA-4795) Merged from 1.1: * Pig: correctly decode row keys in widerow mode (CASSANDRA-5098) http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java index c6adad3..3c30baa 100644 --- a/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java +++ b/src/java/org/apache/cassandra/locator/AbstractReplicationStrategy.java @@ -60,7 +60,7 @@ public abstract class AbstractReplicationStrategy this.tokenMetadata = tokenMetadata; this.snitch = snitch; this.tokenMetadata.register(this); - this.configOptions = configOptions; + this.configOptions = configOptions == null ? Collections.<String, String>emptyMap() : configOptions; this.table = table; } @@ -238,7 +238,12 @@ public abstract class AbstractReplicationStrategy public static Class<AbstractReplicationStrategy> getClass(String cls) throws ConfigurationException { String className = cls.contains(".") ? cls : "org.apache.cassandra.locator." + cls; - return FBUtilities.classForName(className, "replication strategy"); + Class<AbstractReplicationStrategy> strategyClass = FBUtilities.classForName(className, "replication strategy"); + if (!AbstractReplicationStrategy.class.isAssignableFrom(strategyClass)) + { + throw new ConfigurationException(String.format("Specified replication strategy class (%s) is not derived from AbstractReplicationStrategy", className)); + } + return strategyClass; } protected void validateReplicationFactor(String rf) throws ConfigurationException @@ -256,12 +261,12 @@ public abstract class AbstractReplicationStrategy } } - protected void warnOnUnexpectedOptions(Collection<String> expectedOptions) + protected void validateExpectedOptions(Collection<String> expectedOptions) throws ConfigurationException { for (String key : configOptions.keySet()) { if (!expectedOptions.contains(key)) - logger.warn("Unrecognized strategy option {" + key + "} passed to " + getClass().getSimpleName() + " for keyspace " + table); + throw new ConfigurationException(String.format("Unrecognized strategy option {%s} passed to %s for keyspace %s", key, getClass().getSimpleName(), table)); } } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/src/java/org/apache/cassandra/locator/LocalStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/LocalStrategy.java b/src/java/org/apache/cassandra/locator/LocalStrategy.java index 17ad181..4aafa01 100644 --- a/src/java/org/apache/cassandra/locator/LocalStrategy.java +++ b/src/java/org/apache/cassandra/locator/LocalStrategy.java @@ -61,6 +61,6 @@ public class LocalStrategy extends AbstractReplicationStrategy public void validateOptions() throws ConfigurationException { // LocalStrategy doesn't expect any options. - warnOnUnexpectedOptions(Collections.<String>emptySet()); + validateExpectedOptions(Collections.<String>emptySet()); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java index c779dc2..6291da0 100644 --- a/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java +++ b/src/java/org/apache/cassandra/locator/NetworkTopologyStrategy.java @@ -189,6 +189,5 @@ public class NetworkTopologyStrategy extends AbstractReplicationStrategy { validateReplicationFactor(e.getValue()); } - } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/src/java/org/apache/cassandra/locator/SimpleStrategy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/locator/SimpleStrategy.java b/src/java/org/apache/cassandra/locator/SimpleStrategy.java index 7f27334..5d8a723 100644 --- a/src/java/org/apache/cassandra/locator/SimpleStrategy.java +++ b/src/java/org/apache/cassandra/locator/SimpleStrategy.java @@ -68,11 +68,10 @@ public class SimpleStrategy extends AbstractReplicationStrategy public void validateOptions() throws ConfigurationException { - if (configOptions == null || configOptions.get("replication_factor") == null) - { + validateExpectedOptions(Arrays.<String>asList("replication_factor")); + String rf = configOptions.get("replication_factor"); + if (rf == null) throw new ConfigurationException("SimpleStrategy requires a replication_factor strategy option."); - } - warnOnUnexpectedOptions(Arrays.<String>asList("replication_factor")); - validateReplicationFactor(configOptions.get("replication_factor")); + validateReplicationFactor(rf); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/dc94f373/src/java/org/apache/cassandra/thrift/CassandraServer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/thrift/CassandraServer.java b/src/java/org/apache/cassandra/thrift/CassandraServer.java index 49fda60..dfeccb7 100644 --- a/src/java/org/apache/cassandra/thrift/CassandraServer.java +++ b/src/java/org/apache/cassandra/thrift/CassandraServer.java @@ -51,6 +51,7 @@ import org.apache.cassandra.exceptions.ReadTimeoutException; import org.apache.cassandra.exceptions.RequestExecutionException; import org.apache.cassandra.exceptions.RequestValidationException; import org.apache.cassandra.io.util.DataOutputBuffer; +import org.apache.cassandra.locator.AbstractReplicationStrategy; import org.apache.cassandra.locator.DynamicEndpointSnitch; import org.apache.cassandra.scheduler.IRequestScheduler; import org.apache.cassandra.service.*; @@ -1324,6 +1325,13 @@ public class CassandraServer implements Cassandra.Iface state().hasAllKeyspacesAccess(Permission.CREATE); ThriftValidation.validateKeyspaceNotYetExisting(ks_def.name); + // trial run to let ARS validate class + per-class options + AbstractReplicationStrategy.createReplicationStrategy(ks_def.name, + AbstractReplicationStrategy.getClass(ks_def.strategy_class), + StorageService.instance.getTokenMetadata(), + DatabaseDescriptor.getEndpointSnitch(), + ks_def.getStrategy_options()); + // generate a meaningful error if the user setup keyspace and/or column definition incorrectly for (CfDef cf : ks_def.cf_defs) {
