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/trunk Commit: 360d1a2224c8e3614cd665393e0241dc0ba06a58 Parents: ba6cd11 Author: Sylvain Lebresne <[email protected]> Authored: Thu Jan 24 17:53:09 2013 +0100 Committer: Sylvain Lebresne <[email protected]> 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();
