Repository: cassandra Updated Branches: refs/heads/trunk b5dbc04bd -> 4991ca26a
Clean up parsing speculative retry params from string patch by Aleksey Yeschenko; reviewed by Blake Eggleston for CASSANDRA-14352 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/4991ca26 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/4991ca26 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/4991ca26 Branch: refs/heads/trunk Commit: 4991ca26aa424286ebdee89742d35e813f9e9259 Parents: b5dbc04 Author: Aleksey Yeshchenko <alek...@apple.com> Authored: Thu Mar 29 15:37:01 2018 +0100 Committer: Aleksey Yeshchenko <alek...@apple.com> Committed: Tue Apr 10 21:25:16 2018 +0100 ---------------------------------------------------------------------- CHANGES.txt | 4 +- .../apache/cassandra/schema/TableParams.java | 4 +- .../reads/AlwaysSpeculativeRetryPolicy.java | 6 +- .../reads/FixedSpeculativeRetryPolicy.java | 33 ++++- .../reads/HybridSpeculativeRetryPolicy.java | 70 +++++++++-- .../reads/NeverSpeculativeRetryPolicy.java | 6 +- .../reads/PercentileSpeculativeRetryPolicy.java | 45 ++++++- .../service/reads/SpeculativeRetryPolicy.java | 122 +++---------------- 8 files changed, 162 insertions(+), 128 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index c123e6f..650f740 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,12 +1,12 @@ 4.0 + * Add support for hybrid MIN(), MAX() speculative retry policies + (CASSANDRA-14293, CASSANDRA-14338, CASSANDRA-14352) * Fix some regressions caused by 14058 (CASSANDRA-14353) * Abstract repair for pluggable storage (CASSANDRA-14116) * Add meaningful toString() impls (CASSANDRA-13653) * Add sstableloader option to accept target keyspace name (CASSANDRA-13884) * Move processing of EchoMessage response to gossip stage (CASSANDRA-13713) * Add coordinator write metric per CF (CASSANDRA-14232) - * Fix scheduling of speculative retry threshold recalculation (CASSANDRA-14338) - * Add support for hybrid MIN(), MAX() speculative retry policies (CASSANDRA-14293) * Correct and clarify SSLFactory.getSslContext method and call sites (CASSANDRA-14314) * Handle static and partition deletion properly on ThrottledUnfilteredIterator (CASSANDRA-14315) * NodeTool clientstats should show SSL Cipher (CASSANDRA-14322) http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/schema/TableParams.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/schema/TableParams.java b/src/java/org/apache/cassandra/schema/TableParams.java index ffa310e..895e3a7 100644 --- a/src/java/org/apache/cassandra/schema/TableParams.java +++ b/src/java/org/apache/cassandra/schema/TableParams.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import org.apache.cassandra.cql3.Attributes; import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.service.reads.PercentileSpeculativeRetryPolicy; import org.apache.cassandra.service.reads.SpeculativeRetryPolicy; import org.apache.cassandra.utils.BloomCalculations; @@ -70,6 +71,7 @@ public final class TableParams public static final int DEFAULT_MIN_INDEX_INTERVAL = 128; public static final int DEFAULT_MAX_INDEX_INTERVAL = 2048; public static final double DEFAULT_CRC_CHECK_CHANCE = 1.0; + public static final SpeculativeRetryPolicy DEFAULT_SPECULATIVE_RETRY = new PercentileSpeculativeRetryPolicy(99.0); public final String comment; public final double readRepairChance; @@ -290,7 +292,7 @@ public final class TableParams private int memtableFlushPeriodInMs = DEFAULT_MEMTABLE_FLUSH_PERIOD_IN_MS; private int minIndexInterval = DEFAULT_MIN_INDEX_INTERVAL; private int maxIndexInterval = DEFAULT_MAX_INDEX_INTERVAL; - private SpeculativeRetryPolicy speculativeRetry = SpeculativeRetryPolicy.DEFAULT; + private SpeculativeRetryPolicy speculativeRetry = DEFAULT_SPECULATIVE_RETRY; private CachingParams caching = CachingParams.DEFAULT; private CompactionParams compaction = CompactionParams.DEFAULT; private CompressionParams compression = CompressionParams.DEFAULT; http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java index 4623cb1..daf1ec5 100644 --- a/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java @@ -15,7 +15,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; import com.google.common.base.Objects; @@ -59,4 +58,9 @@ public class AlwaysSpeculativeRetryPolicy implements SpeculativeRetryPolicy { return Kind.ALWAYS.toString(); } + + static boolean stringMatches(String str) + { + return str.equalsIgnoreCase("ALWAYS"); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java index 2cd9788..b38b986 100644 --- a/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java @@ -15,20 +15,25 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import com.google.common.base.Objects; import com.codahale.metrics.Timer; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.schema.TableParams; public class FixedSpeculativeRetryPolicy implements SpeculativeRetryPolicy { + private static final Pattern PATTERN = Pattern.compile("^(?<val>[0-9.]+)ms$", Pattern.CASE_INSENSITIVE); + private final int speculateAtMilliseconds; - public FixedSpeculativeRetryPolicy(int speculateAtMilliseconds) + FixedSpeculativeRetryPolicy(int speculateAtMilliseconds) { this.speculateAtMilliseconds = speculateAtMilliseconds; } @@ -65,4 +70,28 @@ public class FixedSpeculativeRetryPolicy implements SpeculativeRetryPolicy { return String.format("%dms", speculateAtMilliseconds); } + + static FixedSpeculativeRetryPolicy fromString(String str) + { + Matcher matcher = PATTERN.matcher(str); + + if (!matcher.matches()) + throw new IllegalArgumentException(); + + String val = matcher.group("val"); + try + { + // historically we've always parsed this as double, but treated as int; so we keep doing it for compatibility + return new FixedSpeculativeRetryPolicy((int) Double.parseDouble(val)); + } + catch (IllegalArgumentException e) + { + throw new ConfigurationException(String.format("Invalid value %s for option '%s'", str, TableParams.Option.SPECULATIVE_RETRY)); + } + } + + static boolean stringMatches(String str) + { + return PATTERN.matcher(str).matches(); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java index d49cfe4..7920ac7 100644 --- a/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java @@ -15,27 +15,40 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + import com.google.common.base.Objects; import com.codahale.metrics.Timer; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.schema.TableParams; public class HybridSpeculativeRetryPolicy implements SpeculativeRetryPolicy { + private static final Pattern PATTERN = + Pattern.compile("^(?<fun>MIN|MAX)\\((?<val1>[0-9.]+[a-z]+)\\s*,\\s*(?<val2>[0-9.]+[a-z]+)\\)$", + Pattern.CASE_INSENSITIVE); + public enum Function { - MIN, MAX + MIN, MAX; + + long call(long val1, long val2) + { + return this == MIN ? Math.min(val1, val2) : Math.max(val1, val2); + } } private final PercentileSpeculativeRetryPolicy percentilePolicy; private final FixedSpeculativeRetryPolicy fixedPolicy; private final Function function; - public HybridSpeculativeRetryPolicy(PercentileSpeculativeRetryPolicy percentilePolicy, - FixedSpeculativeRetryPolicy fixedPolicy, - Function function) + HybridSpeculativeRetryPolicy(PercentileSpeculativeRetryPolicy percentilePolicy, + FixedSpeculativeRetryPolicy fixedPolicy, + Function function) { this.percentilePolicy = percentilePolicy; this.fixedPolicy = fixedPolicy; @@ -45,12 +58,7 @@ public class HybridSpeculativeRetryPolicy implements SpeculativeRetryPolicy @Override public long calculateThreshold(Timer readLatency) { - long percentileThreshold = percentilePolicy.calculateThreshold(readLatency); - long fixedThreshold = fixedPolicy.calculateThreshold(readLatency); - - return function == Function.MIN - ? Math.min(percentileThreshold, fixedThreshold) - : Math.max(percentileThreshold, fixedThreshold); + return function.call(percentilePolicy.calculateThreshold(readLatency), fixedPolicy.calculateThreshold(readLatency)); } @Override @@ -81,4 +89,44 @@ public class HybridSpeculativeRetryPolicy implements SpeculativeRetryPolicy { return String.format("%s(%s,%s)", function, percentilePolicy, fixedPolicy); } + + static HybridSpeculativeRetryPolicy fromString(String str) + { + Matcher matcher = PATTERN.matcher(str); + + if (!matcher.matches()) + throw new IllegalArgumentException(); + + String val1 = matcher.group("val1"); + String val2 = matcher.group("val2"); + + SpeculativeRetryPolicy value1, value2; + try + { + value1 = SpeculativeRetryPolicy.fromString(val1); + value2 = SpeculativeRetryPolicy.fromString(val2); + } + catch (ConfigurationException e) + { + throw new ConfigurationException(String.format("Invalid value %s for option '%s'", str, TableParams.Option.SPECULATIVE_RETRY)); + } + + if (value1.kind() == value2.kind()) + { + throw new ConfigurationException(String.format("Invalid value %s for option '%s': MIN()/MAX() arguments " + + "should be of different types, but both are of type %s", + str, TableParams.Option.SPECULATIVE_RETRY, value1.kind())); + } + + SpeculativeRetryPolicy policy1 = value1 instanceof PercentileSpeculativeRetryPolicy ? value1 : value2; + SpeculativeRetryPolicy policy2 = value1 instanceof FixedSpeculativeRetryPolicy ? value1 : value2; + + Function function = Function.valueOf(matcher.group("fun").toUpperCase()); + return new HybridSpeculativeRetryPolicy((PercentileSpeculativeRetryPolicy) policy1, (FixedSpeculativeRetryPolicy) policy2, function); + } + + static boolean stringMatches(String str) + { + return PATTERN.matcher(str).matches(); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java index c46a899..0b9a861 100644 --- a/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java @@ -15,7 +15,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; import com.google.common.base.Objects; @@ -59,4 +58,9 @@ public class NeverSpeculativeRetryPolicy implements SpeculativeRetryPolicy { return Kind.NEVER.toString(); } + + static boolean stringMatches(String str) + { + return str.equalsIgnoreCase("NEVER") || str.equalsIgnoreCase("NONE"); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java index 172cc0c..b7ccd4c 100644 --- a/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java @@ -15,15 +15,23 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; +import java.text.DecimalFormat; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + import com.google.common.base.Objects; import com.codahale.metrics.Timer; +import org.apache.cassandra.exceptions.ConfigurationException; +import org.apache.cassandra.schema.TableParams; public class PercentileSpeculativeRetryPolicy implements SpeculativeRetryPolicy { + private static final Pattern PATTERN = Pattern.compile("^(?<val>[0-9.]+)p(ercentile)?$", Pattern.CASE_INSENSITIVE); + private static final DecimalFormat FORMATTER = new DecimalFormat("#.####"); + private final double percentile; public PercentileSpeculativeRetryPolicy(double percentile) @@ -61,6 +69,39 @@ public class PercentileSpeculativeRetryPolicy implements SpeculativeRetryPolicy @Override public String toString() { - return String.format("%.2fp", percentile); + return String.format("%sp", FORMATTER.format(percentile)); + } + + static PercentileSpeculativeRetryPolicy fromString(String str) + { + Matcher matcher = PATTERN.matcher(str); + + if (!matcher.matches()) + throw new IllegalArgumentException(); + + String val = matcher.group("val"); + + double percentile; + try + { + percentile = Double.parseDouble(val); + } + catch (IllegalArgumentException e) + { + throw new ConfigurationException(String.format("Invalid value %s for option '%s'", str, TableParams.Option.SPECULATIVE_RETRY)); + } + + if (percentile <= 0.0 || percentile >= 100.0) + { + throw new ConfigurationException(String.format("Invalid value %s for PERCENTILE option '%s': must be between (0.0 and 100.0)", + str, TableParams.Option.SPECULATIVE_RETRY)); + } + + return new PercentileSpeculativeRetryPolicy(percentile); + } + + static boolean stringMatches(String str) + { + return PATTERN.matcher(str).matches(); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/4991ca26/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java b/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java index 225ab26..9bf3a35 100644 --- a/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java +++ b/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java @@ -15,134 +15,40 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.cassandra.service.reads; -import java.util.Locale; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - import com.codahale.metrics.Timer; import org.apache.cassandra.exceptions.ConfigurationException; - -import org.apache.cassandra.service.reads.HybridSpeculativeRetryPolicy.Function; +import org.apache.cassandra.schema.TableParams; public interface SpeculativeRetryPolicy { public enum Kind { - // we need to keep NONE around for legacy tables, but going - // forward we can use the better named NEVER Kind instead - NEVER, NONE, FIXED, PERCENTILE, HYBRID, ALWAYS + NEVER, FIXED, PERCENTILE, HYBRID, ALWAYS } - static final Pattern RETRY_PATTERN = Pattern.compile("^(?<function>MIN|MAX|NEVER|NONE|ALWAYS)?\\(?" + - "(?<val1>[0-9.]+)?(?<op1>P|PERCENTILE|MS)?([,\\s]*)" + - "(?<val2>[0-9.]+)?(?<op2>P|PERCENTILE|MS)?\\)?$", - Pattern.CASE_INSENSITIVE); - public static final SpeculativeRetryPolicy DEFAULT = new PercentileSpeculativeRetryPolicy(99.0); - long calculateThreshold(Timer readLatency); Kind kind(); - public static SpeculativeRetryPolicy nonHybridPolicyFromString(String typeStr, String valueStr) throws ConfigurationException + public static SpeculativeRetryPolicy fromString(String str) { - switch (typeStr.toLowerCase()) - { - case "p": - case "percentile": - { - double value = Double.parseDouble(valueStr); - if (value >= 100.0 || value <= 0) - throw new ConfigurationException(String.format("PERCENTILE should be between 0 and 100 " + - "(not %s)", valueStr)); - return new PercentileSpeculativeRetryPolicy(value); - } - case "ms": - { - // we've always treated this as long, but let's parse as double for compatibility for now - double value = Double.parseDouble(valueStr); - return new FixedSpeculativeRetryPolicy((int) value); - } - default: - throw new ConfigurationException(String.format("invalid speculative_retry type: %s", typeStr)); - } - } + if (AlwaysSpeculativeRetryPolicy.stringMatches(str)) + return AlwaysSpeculativeRetryPolicy.INSTANCE; - public static SpeculativeRetryPolicy fromString(String schemaStrValue) throws ConfigurationException - { - Matcher matcher = RETRY_PATTERN.matcher(schemaStrValue); - if (matcher.find()) - { - String functionStr = matcher.group("function"); - String val1 = matcher.group("val1"); - String op1 = matcher.group("op1"); - String val2 = matcher.group("val2"); - String op2 = matcher.group("op2"); + if (NeverSpeculativeRetryPolicy.stringMatches(str)) + return NeverSpeculativeRetryPolicy.INSTANCE; - String normalizedFunction = (functionStr != null) ? functionStr.toUpperCase(Locale.ENGLISH) : null; - if (normalizedFunction != null && val1 != null && val2 != null) - { - Function hybridFunction; - try - { - hybridFunction = Function.valueOf(normalizedFunction); - } - catch (IllegalArgumentException e) - { - throw new ConfigurationException(String.format("Specified comparator [%s] is not supported", normalizedFunction)); - } + if (PercentileSpeculativeRetryPolicy.stringMatches(str)) + return PercentileSpeculativeRetryPolicy.fromString(str); - SpeculativeRetryPolicy leftPolicy = nonHybridPolicyFromString(op1, val1); - SpeculativeRetryPolicy rightPolicy = nonHybridPolicyFromString(op2, val2); + if (FixedSpeculativeRetryPolicy.stringMatches(str)) + return FixedSpeculativeRetryPolicy.fromString(str); - if (leftPolicy.kind() == rightPolicy.kind()) - { - throw new ConfigurationException(String.format("Speculative Retry Parameters must be " + - "unique types. Both were found to be %s", - leftPolicy.kind().toString())); - } + if (HybridSpeculativeRetryPolicy.stringMatches(str)) + return HybridSpeculativeRetryPolicy.fromString(str); - return (leftPolicy.kind() == Kind.PERCENTILE) - ? new HybridSpeculativeRetryPolicy((PercentileSpeculativeRetryPolicy) leftPolicy, - (FixedSpeculativeRetryPolicy) rightPolicy, hybridFunction) - : new HybridSpeculativeRetryPolicy((PercentileSpeculativeRetryPolicy) rightPolicy, - (FixedSpeculativeRetryPolicy) leftPolicy, hybridFunction); - } - else if (normalizedFunction != null && val1 == null && op1 == null) - { - try - { - Kind kind = Kind.valueOf(normalizedFunction); - switch (kind) - { - case NONE: - case NEVER: return NeverSpeculativeRetryPolicy.INSTANCE; - case ALWAYS: return AlwaysSpeculativeRetryPolicy.INSTANCE; - default: throw new ConfigurationException(String.format("Specified comparator [%s] is not supported", normalizedFunction)); - } - } - catch (IllegalArgumentException e) - { - throw new ConfigurationException(String.format("Specified comparator [%s] is not supported", normalizedFunction)); - } - } - else - { - if (op1 == null || val1 == null) - { - throw new ConfigurationException(String.format("Specified Speculative Retry Policy [%s] is not supported", schemaStrValue)); - } - else - { - return nonHybridPolicyFromString(op1, val1); - } - } - } - else - { - throw new ConfigurationException(String.format("Specified Speculative Retry Policy [%s] is not supported", schemaStrValue)); - } + throw new ConfigurationException(String.format("Invalid value %s for option '%s'", str, TableParams.Option.SPECULATIVE_RETRY)); } } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
