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)
             {

Reply via email to