You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sl...@apache.org on 2013/01/24 17:57:19 UTC
git commit: Validate compaction strategy options
Updated Branches:
refs/heads/cassandra-1.2 ba6cd11b7 -> 360d1a222
Validate compaction 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/360d1a22
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/360d1a22
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/360d1a22
Branch: refs/heads/cassandra-1.2
Commit: 360d1a2224c8e3614cd665393e0241dc0ba06a58
Parents: ba6cd11
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Thu Jan 24 17:53:09 2013 +0100
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Thu Jan 24 17:53:09 2013 +0100
----------------------------------------------------------------------
CHANGES.txt | 2 +-
NEWS.txt | 2 +
.../org/apache/cassandra/config/CFMetaData.java | 35 ++++++++-
src/java/org/apache/cassandra/cql/CFPropDefs.java | 4 +-
.../cassandra/cql/CreateColumnFamilyStatement.java | 3 +-
src/java/org/apache/cassandra/cql3/CFPropDefs.java | 7 ++
.../db/compaction/AbstractCompactionStrategy.java | 67 +++++++++++++--
.../db/compaction/LeveledCompactionStrategy.java | 39 ++++++---
.../compaction/SizeTieredCompactionStrategy.java | 62 ++++++++++++--
.../apache/cassandra/thrift/CassandraServer.java | 3 +
10 files changed, 192 insertions(+), 32 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 02317b1..812abdd 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -30,7 +30,7 @@
* Validate correctly selects on composite partition key (CASSANDRA-5122)
* Fix exception when adding collection (CASSANDRA-5117)
* Handle states for non-vnode clusters correctly (CASSANDRA-5127)
- * Refuse unrecognized replication strategy options (CASSANDRA-4795)
+ * Refuse unrecognized replication and compaction strategy options (CASSANDRA-4795)
* Pick the correct value validator in sstable2json for cql3 tables (CASSANDRA-5134)
* Validate login for describe_keyspace, describe_keyspaces and set_keyspace
(CASSANDRA-5144)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/NEWS.txt
----------------------------------------------------------------------
diff --git a/NEWS.txt b/NEWS.txt
index 072b647..c6757aa 100644
--- a/NEWS.txt
+++ b/NEWS.txt
@@ -26,6 +26,8 @@ Upgrading
since 1.2.0. However, Cassandra 1.2.0 was not complaining if CQL3 was set
through set_cql_version but the now CQL2 only methods were used. This is
now the case.
+ - Queries that uses unrecognized or bad compaction or replication strategy
+ options are now refused (instead of simply logging a warning).
1.2
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/config/CFMetaData.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/config/CFMetaData.java b/src/java/org/apache/cassandra/config/CFMetaData.java
index 82d49a9..c829fa3 100644
--- a/src/java/org/apache/cassandra/config/CFMetaData.java
+++ b/src/java/org/apache/cassandra/config/CFMetaData.java
@@ -19,6 +19,7 @@ package org.apache.cassandra.config;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
import java.nio.ByteBuffer;
import java.util.*;
@@ -838,10 +839,42 @@ public final class CFMetaData
throw new ConfigurationException("subcolumncomparators do not match or are note compatible.");
}
+ public static void validateCompactionOptions(Class<? extends AbstractCompactionStrategy> strategyClass, Map<String, String> options) throws ConfigurationException
+ {
+ try
+ {
+ if (options == null)
+ return;
+
+ Method validateMethod = strategyClass.getMethod("validateOptions", Map.class);
+ Map<String, String> unknownOptions = (Map<String, String>) validateMethod.invoke(null, options);
+ if (!unknownOptions.isEmpty())
+ throw new ConfigurationException(String.format("Properties specified %s are not understood by %s", unknownOptions.keySet(), strategyClass.getSimpleName()));
+ }
+ catch (NoSuchMethodException e)
+ {
+ logger.warn("Compaction Strategy {} does not have a static validateOptions method. Validation ignored", strategyClass.getName());
+ }
+ catch (InvocationTargetException e)
+ {
+ if (e.getTargetException() instanceof ConfigurationException)
+ throw (ConfigurationException) e.getTargetException();
+ throw new ConfigurationException("Failed to validate compaction options");
+ }
+ catch (Exception e)
+ {
+ throw new ConfigurationException("Failed to validate compaction options");
+ }
+ }
+
public static Class<? extends AbstractCompactionStrategy> createCompactionStrategy(String className) throws ConfigurationException
{
className = className.contains(".") ? className : "org.apache.cassandra.db.compaction." + className;
- return FBUtilities.classForName(className, "compaction strategy");
+ Class<AbstractCompactionStrategy> strategyClass = FBUtilities.classForName(className, "compaction strategy");
+ if (!AbstractCompactionStrategy.class.isAssignableFrom(strategyClass))
+ throw new ConfigurationException(String.format("Specified compaction strategy class (%s) is not derived from AbstractReplicationStrategy", className));
+
+ return strategyClass;
}
public AbstractCompactionStrategy createCompactionStrategyInstance(ColumnFamilyStore cfs)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/cql/CFPropDefs.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql/CFPropDefs.java b/src/java/org/apache/cassandra/cql/CFPropDefs.java
index ff9ebad..b488ecf 100644
--- a/src/java/org/apache/cassandra/cql/CFPropDefs.java
+++ b/src/java/org/apache/cassandra/cql/CFPropDefs.java
@@ -107,7 +107,7 @@ public class CFPropDefs {
public final Map<String, String> compactionStrategyOptions = new HashMap<String, String>();
public final Map<String, String> compressionParameters = new HashMap<String, String>();
- public void validate() throws InvalidRequestException
+ public void validate() throws InvalidRequestException, ConfigurationException
{
compactionStrategyClass = CFMetaData.DEFAULT_COMPACTION_STRATEGY_CLASS;
@@ -171,6 +171,8 @@ public class CFPropDefs {
KW_MINCOMPACTIONTHRESHOLD,
CFMetaData.DEFAULT_MIN_COMPACTION_THRESHOLD));
}
+
+ CFMetaData.validateCompactionOptions(compactionStrategyClass, compactionStrategyOptions);
}
/** Map a keyword to the corresponding value */
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java b/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java
index a84f93e..41fb291 100644
--- a/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java
+++ b/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java
@@ -52,8 +52,6 @@ public class CreateColumnFamilyStatement
/** Perform validation of parsed params */
private void validate(List<ByteBuffer> variables) throws InvalidRequestException
{
- cfProps.validate();
-
// Ensure that exactly one key has been specified.
if (keyValidator.size() < 1)
throw new InvalidRequestException("You must specify a PRIMARY KEY");
@@ -64,6 +62,7 @@ public class CreateColumnFamilyStatement
try
{
+ cfProps.validate();
comparator = cfProps.getComparator();
}
catch (ConfigurationException e)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/cql3/CFPropDefs.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/CFPropDefs.java b/src/java/org/apache/cassandra/cql3/CFPropDefs.java
index 2cbcfde..c546cfc 100644
--- a/src/java/org/apache/cassandra/cql3/CFPropDefs.java
+++ b/src/java/org/apache/cassandra/cql3/CFPropDefs.java
@@ -85,9 +85,16 @@ public class CFPropDefs extends PropertyDefinitions
compactionStrategyClass = CFMetaData.createCompactionStrategy(strategy);
compactionOptions.remove(COMPACTION_STRATEGY_CLASS_KEY);
+
+ CFMetaData.validateCompactionOptions(compactionStrategyClass, compactionOptions);
}
}
+ public Class<? extends AbstractCompactionStrategy> getCompactionStrategy()
+ {
+ return compactionStrategyClass;
+ }
+
public Map<String, String> getCompactionOptions() throws SyntaxException
{
Map<String, String> compactionOptions = getMap(KW_COMPACTION);
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java b/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java
index 94743f9..066f2f3 100644
--- a/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java
+++ b/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java
@@ -25,9 +25,12 @@ import org.slf4j.LoggerFactory;
import org.apache.cassandra.db.ColumnFamilyStore;
import org.apache.cassandra.dht.Range;
import org.apache.cassandra.dht.Token;
+import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.io.sstable.Component;
import org.apache.cassandra.io.sstable.SSTableReader;
+import com.google.common.collect.Sets;
+
/**
* Pluggable compaction strategy determines how SSTables get merged.
*
@@ -49,7 +52,7 @@ public abstract class AbstractCompactionStrategy
public final Map<String, String> options;
protected final ColumnFamilyStore cfs;
- protected final float tombstoneThreshold;
+ protected float tombstoneThreshold;
protected long tombstoneCompactionInterval;
protected AbstractCompactionStrategy(ColumnFamilyStore cfs, Map<String, String> options)
@@ -58,14 +61,20 @@ public abstract class AbstractCompactionStrategy
this.cfs = cfs;
this.options = options;
- String optionValue = options.get(TOMBSTONE_THRESHOLD_OPTION);
- tombstoneThreshold = optionValue == null ? DEFAULT_TOMBSTONE_THRESHOLD : Float.parseFloat(optionValue);
- optionValue = options.get(TOMBSTONE_COMPACTION_INTERVAL_OPTION);
- tombstoneCompactionInterval = optionValue == null ? DEFAULT_TOMBSTONE_COMPACTION_INTERVAL : Long.parseLong(optionValue);
- if (tombstoneCompactionInterval < 0)
+ /* checks must be repeated here, as user supplied strategies might not call validateOptions directly */
+
+ try
+ {
+ validateOptions(options);
+ String optionValue = options.get(TOMBSTONE_THRESHOLD_OPTION);
+ tombstoneThreshold = optionValue == null ? DEFAULT_TOMBSTONE_THRESHOLD : Float.parseFloat(optionValue);
+ optionValue = options.get(TOMBSTONE_COMPACTION_INTERVAL_OPTION);
+ tombstoneCompactionInterval = optionValue == null ? DEFAULT_TOMBSTONE_COMPACTION_INTERVAL : Long.parseLong(optionValue);
+ }
+ catch (ConfigurationException e)
{
- logger.warn("tombstone_compaction_interval should not be negative({}). Using default value of {}.",
- tombstoneCompactionInterval, DEFAULT_TOMBSTONE_COMPACTION_INTERVAL);
+ logger.warn("Error setting compaction strategy options ({}), defaults will be used", e.getMessage());
+ tombstoneThreshold = DEFAULT_TOMBSTONE_THRESHOLD;
tombstoneCompactionInterval = DEFAULT_TOMBSTONE_COMPACTION_INTERVAL;
}
}
@@ -194,4 +203,46 @@ public abstract class AbstractCompactionStrategy
return remainingColumnsRatio * droppableRatio > tombstoneThreshold;
}
}
+
+ public static Map<String, String> validateOptions(Map<String, String> options) throws ConfigurationException
+ {
+ String threshold = options.get(TOMBSTONE_THRESHOLD_OPTION);
+ if (threshold != null)
+ {
+ try
+ {
+ float thresholdValue = Float.parseFloat(threshold);
+ if (thresholdValue < 0)
+ {
+ throw new ConfigurationException(String.format("%s must be greater than 0, but was %d", TOMBSTONE_THRESHOLD_OPTION, thresholdValue));
+ }
+ }
+ catch (NumberFormatException e)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", threshold, TOMBSTONE_THRESHOLD_OPTION), e);
+ }
+ }
+
+ String interval = options.get(TOMBSTONE_COMPACTION_INTERVAL_OPTION);
+ if (interval != null)
+ {
+ try
+ {
+ long tombstoneCompactionInterval = Long.parseLong(interval);
+ if (tombstoneCompactionInterval < 0)
+ {
+ throw new ConfigurationException(String.format("%s must be greater than 0, but was %d", TOMBSTONE_COMPACTION_INTERVAL_OPTION, tombstoneCompactionInterval));
+ }
+ }
+ catch (NumberFormatException e)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", interval, TOMBSTONE_COMPACTION_INTERVAL_OPTION), e);
+ }
+ }
+
+ Map<String, String> uncheckedOptions = new HashMap<String, String>(options);
+ uncheckedOptions.remove(TOMBSTONE_THRESHOLD_OPTION);
+ uncheckedOptions.remove(TOMBSTONE_COMPACTION_INTERVAL_OPTION);
+ return uncheckedOptions;
+ }
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java b/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java
index 1522d18..fe5daf5 100644
--- a/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java
+++ b/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java
@@ -31,6 +31,7 @@ import org.apache.cassandra.db.ColumnFamilyStore;
import org.apache.cassandra.db.columniterator.OnDiskAtomIterator;
import org.apache.cassandra.dht.Range;
import org.apache.cassandra.dht.Token;
+import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.io.sstable.SSTable;
import org.apache.cassandra.io.sstable.SSTableReader;
import org.apache.cassandra.io.sstable.SSTableScanner;
@@ -54,19 +55,8 @@ public class LeveledCompactionStrategy extends AbstractCompactionStrategy implem
int configuredMaxSSTableSize = 5;
if (options != null)
{
- String value = options.containsKey(SSTABLE_SIZE_OPTION) ? options.get(SSTABLE_SIZE_OPTION) : null;
- if (value != null)
- {
- try
- {
- configuredMaxSSTableSize = Integer.parseInt(value);
- }
- catch (NumberFormatException ex)
- {
- logger.warn(String.format("%s is not a parsable int (base10) for %s using default value",
- value, SSTABLE_SIZE_OPTION));
- }
- }
+ String value = options.containsKey(SSTABLE_SIZE_OPTION) ? options.get(SSTABLE_SIZE_OPTION) : "5";
+ configuredMaxSSTableSize = Integer.parseInt(value);
}
maxSSTableSizeInMB = configuredMaxSSTableSize;
@@ -309,4 +299,27 @@ public class LeveledCompactionStrategy extends AbstractCompactionStrategy implem
}
return null;
}
+
+ public static Map<String, String> validateOptions(Map<String, String> options) throws ConfigurationException
+ {
+ Map<String, String> uncheckedOptions = AbstractCompactionStrategy.validateOptions(options);
+
+ String size = options.containsKey(SSTABLE_SIZE_OPTION) ? options.get(SSTABLE_SIZE_OPTION) : "1";
+ try
+ {
+ int ssSize = Integer.parseInt(size);
+ if (ssSize < 1)
+ {
+ throw new ConfigurationException(String.format("%s must be larger than 0, but was %s", SSTABLE_SIZE_OPTION, ssSize));
+ }
+ }
+ catch (NumberFormatException ex)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", size, SSTABLE_SIZE_OPTION), ex);
+ }
+
+ uncheckedOptions.remove(SSTABLE_SIZE_OPTION);
+
+ return uncheckedOptions;
+ }
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java b/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java
index 7fc9f13..64ed744 100644
--- a/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java
+++ b/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java
@@ -23,7 +23,9 @@ import java.util.Map.Entry;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import org.apache.cassandra.cql3.CFPropDefs;
import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.io.sstable.SSTableReader;
import org.apache.cassandra.utils.Pair;
@@ -52,12 +54,7 @@ public class SizeTieredCompactionStrategy extends AbstractCompactionStrategy
bucketLow = optionValue == null ? DEFAULT_BUCKET_LOW : Double.parseDouble(optionValue);
optionValue = options.get(BUCKET_HIGH_KEY);
bucketHigh = optionValue == null ? DEFAULT_BUCKET_HIGH : Double.parseDouble(optionValue);
- if (bucketHigh <= bucketLow)
- {
- logger.warn("Bucket low/high marks for {} incorrect, using defaults.", cfs.getColumnFamilyName());
- bucketLow = DEFAULT_BUCKET_LOW;
- bucketHigh = DEFAULT_BUCKET_HIGH;
- }
+
cfs.setCompactionThresholds(cfs.metadata.getMinCompactionThreshold(), cfs.metadata.getMaxCompactionThreshold());
}
@@ -227,6 +224,59 @@ public class SizeTieredCompactionStrategy extends AbstractCompactionStrategy
return Long.MAX_VALUE;
}
+ public static Map<String, String> validateOptions(Map<String, String> options) throws ConfigurationException
+ {
+ Map<String, String> uncheckedOptions = AbstractCompactionStrategy.validateOptions(options);
+
+ String optionValue = options.get(MIN_SSTABLE_SIZE_KEY);
+ try
+ {
+ long minSSTableSize = optionValue == null ? DEFAULT_MIN_SSTABLE_SIZE : Long.parseLong(optionValue);
+ if (minSSTableSize < 0)
+ {
+ throw new ConfigurationException(String.format("%s must be non negative: %d", MIN_SSTABLE_SIZE_KEY, minSSTableSize));
+ }
+ }
+ catch (NumberFormatException e)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", optionValue, MIN_SSTABLE_SIZE_KEY), e);
+ }
+
+ double bucketLow, bucketHigh;
+ optionValue = options.get(BUCKET_LOW_KEY);
+ try
+ {
+ bucketLow = optionValue == null ? DEFAULT_BUCKET_LOW : Double.parseDouble(optionValue);
+ }
+ catch (NumberFormatException e)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", optionValue, DEFAULT_BUCKET_LOW), e);
+ }
+
+ optionValue = options.get(BUCKET_HIGH_KEY);
+ try
+ {
+ bucketHigh = optionValue == null ? DEFAULT_BUCKET_HIGH : Double.parseDouble(optionValue);
+ }
+ catch (NumberFormatException e)
+ {
+ throw new ConfigurationException(String.format("%s is not a parsable int (base10) for %s", optionValue, DEFAULT_BUCKET_HIGH), e);
+ }
+
+ if (bucketHigh <= bucketLow)
+ {
+ throw new ConfigurationException(String.format("BucketHigh value (%s) is less than or equal BucketLow value (%s)", bucketHigh, bucketLow));
+ }
+
+ uncheckedOptions.remove(MIN_SSTABLE_SIZE_KEY);
+ uncheckedOptions.remove(BUCKET_LOW_KEY);
+ uncheckedOptions.remove(BUCKET_HIGH_KEY);
+ uncheckedOptions.remove(CFPropDefs.KW_MINCOMPACTIONTHRESHOLD);
+ uncheckedOptions.remove(CFPropDefs.KW_MAXCOMPACTIONTHRESHOLD);
+
+ return uncheckedOptions;
+ }
+
public String toString()
{
return String.format("SizeTieredCompactionStrategy[%s/%s]",
http://git-wip-us.apache.org/repos/asf/cassandra/blob/360d1a22/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 557ba2c..fbdf184 100644
--- a/src/java/org/apache/cassandra/thrift/CassandraServer.java
+++ b/src/java/org/apache/cassandra/thrift/CassandraServer.java
@@ -1301,6 +1301,8 @@ public class CassandraServer implements Cassandra.Iface
cState.hasKeyspaceAccess(keyspace, Permission.CREATE);
cf_def.unsetId(); // explicitly ignore any id set by client (Hector likes to set zero)
CFMetaData cfm = CFMetaData.fromThrift(cf_def);
+ CFMetaData.validateCompactionOptions(cfm.compactionStrategyClass, cfm.compactionStrategyOptions);
+
cfm.addDefaultIndexNames();
MigrationManager.announceNewColumnFamily(cfm);
return Schema.instance.getVersion().toString();
@@ -1436,6 +1438,7 @@ public class CassandraServer implements Cassandra.Iface
CFMetaData.applyImplicitDefaults(cf_def);
CFMetaData cfm = CFMetaData.fromThrift(cf_def);
+ CFMetaData.validateCompactionOptions(cfm.compactionStrategyClass, cfm.compactionStrategyOptions);
cfm.addDefaultIndexNames();
MigrationManager.announceColumnFamilyUpdate(cfm);
return Schema.instance.getVersion().toString();