You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by aw...@apache.org on 2018/09/07 19:13:28 UTC
cassandra git commit:
DecayingEstimatedHistogramReservoir.EstimatedHistogramReservoirSnapshot
returns wrong value for size() and incorrectly calculates count
Repository: cassandra
Updated Branches:
refs/heads/trunk ab2faa8a4 -> 8d443805f
DecayingEstimatedHistogramReservoir.EstimatedHistogramReservoirSnapshot returns wrong value for size() and incorrectly calculates count
Patch by Ariel Weisberg; Reviewed by Chris Lohfink for CASSANDRA-14696
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/8d443805
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/8d443805
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/8d443805
Branch: refs/heads/trunk
Commit: 8d443805f06e7abb25f768f6c800b7ae71bd4a41
Parents: ab2faa8
Author: Ariel Weisberg <aw...@apple.com>
Authored: Wed Sep 5 17:35:47 2018 -0400
Committer: Ariel Weisberg <aw...@apple.com>
Committed: Fri Sep 7 15:11:59 2018 -0400
----------------------------------------------------------------------
CHANGES.txt | 1 +
.../apache/cassandra/db/ColumnFamilyStore.java | 5 +++--
.../DecayingEstimatedHistogramReservoir.java | 20 +++++++++-----------
.../reads/AlwaysSpeculativeRetryPolicy.java | 4 ++--
.../reads/FixedSpeculativeRetryPolicy.java | 4 ++--
.../reads/HybridSpeculativeRetryPolicy.java | 7 +++++--
.../reads/NeverSpeculativeRetryPolicy.java | 4 ++--
.../reads/PercentileSpeculativeRetryPolicy.java | 8 +++++---
.../service/reads/SpeculativeRetryPolicy.java | 4 ++--
...DecayingEstimatedHistogramReservoirTest.java | 11 +++++++++++
10 files changed, 42 insertions(+), 26 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 2fd02e2..b7bc775 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
4.0
+ * DecayingEstimatedHistogramReservoir.EstimatedHistogramReservoirSnapshot returns wrong value for size() and incorrectly calculates count (CASSANDRA-14696)
* AbstractReplicaCollection equals and hash code should throw due to conflict between order sensitive/insensitive uses (CASSANDRA-14700)
* Detect inconsistencies in repaired data on the read path (CASSANDRA-14145)
* Add checksumming to the native protocol (CASSANDRA-13304)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
index 56851e2..bc69026 100644
--- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
+++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
@@ -41,6 +41,7 @@ import com.google.common.util.concurrent.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.codahale.metrics.Snapshot;
import org.apache.cassandra.cache.*;
import org.apache.cassandra.concurrent.*;
import org.apache.cassandra.config.*;
@@ -453,8 +454,8 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean
{
try
{
- sampleReadLatencyNanos = metadata().params.speculativeRetry.calculateThreshold(metric.coordinatorReadLatency);
- transientWriteLatencyNanos = metadata().params.speculativeWriteThreshold.calculateThreshold(metric.coordinatorWriteLatency);
+ sampleReadLatencyNanos = metadata().params.speculativeRetry.calculateThreshold(metric.coordinatorReadLatency.getSnapshot(), sampleReadLatencyNanos);
+ transientWriteLatencyNanos = metadata().params.speculativeWriteThreshold.calculateThreshold(metric.coordinatorWriteLatency.getSnapshot(), transientWriteLatencyNanos);
}
catch (Throwable e)
{
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java b/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java
index f17de78..a3168cf 100644
--- a/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java
+++ b/src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java
@@ -27,6 +27,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.LongAdder;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.primitives.Ints;
import com.codahale.metrics.Clock;
import com.codahale.metrics.Reservoir;
@@ -316,7 +317,7 @@ public class DecayingEstimatedHistogramReservoir implements Reservoir
* The decaying buckets will be used for quantile calculations and mean values, but the non decaying buckets will be
* exposed for calls to {@link Snapshot#getValues()}.
*/
- class EstimatedHistogramReservoirSnapshot extends Snapshot
+ static class EstimatedHistogramReservoirSnapshot extends Snapshot
{
private final long[] decayingBuckets;
private final long[] values;
@@ -328,19 +329,19 @@ public class DecayingEstimatedHistogramReservoir implements Reservoir
public EstimatedHistogramReservoirSnapshot(DecayingEstimatedHistogramReservoir reservoir)
{
final int length = reservoir.decayingBuckets.length;
- final double rescaleFactor = forwardDecayWeight(clock.getTime());
+ final double rescaleFactor = reservoir.forwardDecayWeight(reservoir.clock.getTime());
this.decayingBuckets = new long[length];
this.values = new long[length];
- this.count = count();
- this.snapshotLandmark = decayLandmark;
+ this.snapshotLandmark = reservoir.decayLandmark;
this.bucketOffsets = reservoir.bucketOffsets; // No need to copy, these are immutable
for (int i = 0; i < length; i++)
{
this.decayingBuckets[i] = Math.round(reservoir.decayingBuckets[i].sum() / rescaleFactor);
- this.values[i] = buckets[i].sum();
+ this.values[i] = reservoir.buckets[i].sum();
}
+ this.count = count();
this.reservoir = reservoir;
}
@@ -388,15 +389,12 @@ public class DecayingEstimatedHistogramReservoir implements Reservoir
}
/**
- * Return the number of buckets where recorded values are stored.
- *
- * This method does not return the number of recorded values as suggested by the {@link Snapshot} interface.
- *
- * @return the number of buckets
+ * @see {@link Snapshot#size()}
+ * @return
*/
public int size()
{
- return decayingBuckets.length;
+ return Ints.saturatedCast(count);
}
@VisibleForTesting
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 daf1ec5..a6092fb 100644
--- a/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/AlwaysSpeculativeRetryPolicy.java
@@ -19,7 +19,7 @@ package org.apache.cassandra.service.reads;
import com.google.common.base.Objects;
-import com.codahale.metrics.Timer;
+import com.codahale.metrics.Snapshot;
public class AlwaysSpeculativeRetryPolicy implements SpeculativeRetryPolicy
{
@@ -30,7 +30,7 @@ public class AlwaysSpeculativeRetryPolicy implements SpeculativeRetryPolicy
}
@Override
- public long calculateThreshold(Timer readLatency)
+ public long calculateThreshold(Snapshot latency, long existingValue)
{
return 0;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 b38b986..9bbeb12 100644
--- a/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/FixedSpeculativeRetryPolicy.java
@@ -23,7 +23,7 @@ import java.util.regex.Pattern;
import com.google.common.base.Objects;
-import com.codahale.metrics.Timer;
+import com.codahale.metrics.Snapshot;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.schema.TableParams;
@@ -39,7 +39,7 @@ public class FixedSpeculativeRetryPolicy implements SpeculativeRetryPolicy
}
@Override
- public long calculateThreshold(Timer readLatency)
+ public long calculateThreshold(Snapshot latency, long existingValue)
{
return TimeUnit.MILLISECONDS.toNanos(speculateAtMilliseconds);
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 7920ac7..8228c45 100644
--- a/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/HybridSpeculativeRetryPolicy.java
@@ -22,6 +22,7 @@ import java.util.regex.Pattern;
import com.google.common.base.Objects;
+import com.codahale.metrics.Snapshot;
import com.codahale.metrics.Timer;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.schema.TableParams;
@@ -56,9 +57,11 @@ public class HybridSpeculativeRetryPolicy implements SpeculativeRetryPolicy
}
@Override
- public long calculateThreshold(Timer readLatency)
+ public long calculateThreshold(Snapshot latency, long existingValue)
{
- return function.call(percentilePolicy.calculateThreshold(readLatency), fixedPolicy.calculateThreshold(readLatency));
+ if (latency.size() <= 0)
+ return existingValue;
+ return function.call(percentilePolicy.calculateThreshold(latency, existingValue), fixedPolicy.calculateThreshold(latency, existingValue));
}
@Override
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 0b9a861..1211142 100644
--- a/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/NeverSpeculativeRetryPolicy.java
@@ -19,7 +19,7 @@ package org.apache.cassandra.service.reads;
import com.google.common.base.Objects;
-import com.codahale.metrics.Timer;
+import com.codahale.metrics.Snapshot;
public class NeverSpeculativeRetryPolicy implements SpeculativeRetryPolicy
{
@@ -30,7 +30,7 @@ public class NeverSpeculativeRetryPolicy implements SpeculativeRetryPolicy
}
@Override
- public long calculateThreshold(Timer readLatency)
+ public long calculateThreshold(Snapshot latency, long existingValue)
{
return Long.MAX_VALUE;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 42f90fe..ffd473e 100644
--- a/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/PercentileSpeculativeRetryPolicy.java
@@ -25,7 +25,7 @@ import java.util.regex.Pattern;
import com.google.common.base.Objects;
-import com.codahale.metrics.Timer;
+import com.codahale.metrics.Snapshot;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.schema.TableParams;
@@ -47,9 +47,11 @@ public class PercentileSpeculativeRetryPolicy implements SpeculativeRetryPolicy
}
@Override
- public long calculateThreshold(Timer readLatency)
+ public long calculateThreshold(Snapshot latency, long existingValue)
{
- return (long) readLatency.getSnapshot().getValue(percentile / 100);
+ if (latency.size() <= 0)
+ return existingValue;
+ return (long) latency.getValue(percentile / 100);
}
@Override
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/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 9bf3a35..e09ff51 100644
--- a/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java
+++ b/src/java/org/apache/cassandra/service/reads/SpeculativeRetryPolicy.java
@@ -17,7 +17,7 @@
*/
package org.apache.cassandra.service.reads;
-import com.codahale.metrics.Timer;
+import com.codahale.metrics.Snapshot;
import org.apache.cassandra.exceptions.ConfigurationException;
import org.apache.cassandra.schema.TableParams;
@@ -28,7 +28,7 @@ public interface SpeculativeRetryPolicy
NEVER, FIXED, PERCENTILE, HYBRID, ALWAYS
}
- long calculateThreshold(Timer readLatency);
+ long calculateThreshold(Snapshot latency, long existingValue);
Kind kind();
http://git-wip-us.apache.org/repos/asf/cassandra/blob/8d443805/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTest.java b/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTest.java
index 5cfd927..4a9d18b 100644
--- a/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTest.java
+++ b/test/unit/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoirTest.java
@@ -415,6 +415,17 @@ public class DecayingEstimatedHistogramReservoirTest
assertEquals(2500, snapshot.getMean(), 500D);
}
+ @Test
+ public void testSize()
+ {
+ TestClock clock = new TestClock();
+
+ DecayingEstimatedHistogramReservoir histogram = new DecayingEstimatedHistogramReservoir(DecayingEstimatedHistogramReservoir.DEFAULT_ZERO_CONSIDERATION, DecayingEstimatedHistogramReservoir.DEFAULT_BUCKET_COUNT, clock);
+ histogram.update(42);
+ histogram.update(42);
+ assertEquals(2, histogram.getSnapshot().size());
+ }
+
private void assertEstimatedQuantile(long expectedValue, double actualValue)
{
assertTrue("Expected at least [" + expectedValue + "] but actual is [" + actualValue + "]", actualValue >= expectedValue);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org