diff --git a/CHANGES.txt b/CHANGES.txt index 9c3ea06635..2ba47bb462 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -127,6 +127,7 @@ Merged from 4.1: * Fix StackOverflowError on ALTER after many previous schema changes (CASSANDRA-19166) * Memoize Cassandra verion (CASSANDRA-18902) Merged from 4.0: + * Fix latency reported by ideal consistency level monitoring (CASSANDRA-19651) * Use default commitlog settings in test YAMLs (CASSANDRA-19830) * Do not spam log with SSLExceptions (CASSANDRA-18839) * Fix schema.cql created by a snapshot after dropping more than one column (CASSANDRA-19747) diff --git a/src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java b/src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java index d3d0572f10..4ebca9a232 100644 --- a/src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java +++ b/src/java/org/apache/cassandra/service/AbstractWriteResponseHandler.java @@ -200,7 +200,7 @@ public abstract class AbstractWriteResponseHandler implements RequestCallback } } - public final void expired() + protected final void logFailureOrTimeoutToIdealCLDelegate() { //Tracking ideal CL was not configured if (idealCLDelegate == null) @@ -220,6 +220,11 @@ public abstract class AbstractWriteResponseHandler implements RequestCallback } } + public final void expired() + { + logFailureOrTimeoutToIdealCLDelegate(); + } + /** * @return the minimum number of endpoints that must respond. */ @@ -270,9 +275,13 @@ public abstract class AbstractWriteResponseHandler implements RequestCallback { //The ideal CL should only count as a strike if the requested CL was achieved. //If the requested CL is not achieved it's fine for the ideal CL to also not be achieved. - if (idealCLDelegate != null) + if (idealCLDelegate != null && blockFor() + failures <= candidateReplicaCount()) { idealCLDelegate.requestedCLAchieved = true; + if (idealCLDelegate == this) + { + replicaPlan.keyspace().metric.idealCLWriteLatency.addNano(nanoTime() - requestTime.startedAtNanos()); + } } condition.signalAll(); @@ -291,6 +300,8 @@ public abstract class AbstractWriteResponseHandler implements RequestCallback failureReasonByEndpoint.put(from, failureReason); + logFailureOrTimeoutToIdealCLDelegate(); + if (blockFor() + n > candidateReplicaCount()) signal(); @@ -320,10 +331,6 @@ public abstract class AbstractWriteResponseHandler implements RequestCallback { replicaPlan.keyspace().metric.writeFailedIdealCL.inc(); } - else - { - replicaPlan.keyspace().metric.idealCLWriteLatency.addNano(nanoTime() - requestTime.startedAtNanos()); - } } } diff --git a/test/unit/org/apache/cassandra/service/WriteResponseHandlerTest.java b/test/unit/org/apache/cassandra/service/WriteResponseHandlerTest.java index af992e714c..6356f3ce1a 100644 --- a/test/unit/org/apache/cassandra/service/WriteResponseHandlerTest.java +++ b/test/unit/org/apache/cassandra/service/WriteResponseHandlerTest.java @@ -35,6 +35,7 @@ import org.apache.cassandra.db.ConsistencyLevel; import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.WriteType; import org.apache.cassandra.dht.Murmur3Partitioner; +import org.apache.cassandra.exceptions.RequestFailureReason; import org.apache.cassandra.locator.EndpointsForToken; import org.apache.cassandra.locator.IEndpointSnitch; import org.apache.cassandra.locator.InetAddressAndPort; @@ -151,17 +152,23 @@ public class WriteResponseHandlerTest //dc1 awr.onResponse(createDummyMessage(0)); awr.onResponse(createDummyMessage(1)); + + // there are not enough responses for ideal EACH_QUORUM yet + assertEquals(startingCount, ks.metric.idealCLWriteLatency.latency.getCount()); + //dc2 awr.onResponse(createDummyMessage(4)); awr.onResponse(createDummyMessage(5)); + // there are enough responses for ideal EACH_QUORUM, we should not wait for all responses + assertTrue( TimeUnit.DAYS.toMicros(1) < ks.metric.idealCLWriteLatency.totalLatency.getCount()); + assertEquals(startingCount + 1, ks.metric.idealCLWriteLatency.latency.getCount()); + //Don't need the others awr.expired(); awr.expired(); assertEquals(0, ks.metric.writeFailedIdealCL.getCount()); - assertTrue( TimeUnit.DAYS.toMicros(1) < ks.metric.idealCLWriteLatency.totalLatency.getCount()); - assertEquals(startingCount + 1, ks.metric.idealCLWriteLatency.latency.getCount()); } /** @@ -233,6 +240,30 @@ public class WriteResponseHandlerTest assertEquals(0, ks.metric.idealCLWriteLatency.totalLatency.getCount()); } + @Test + public void failedIdealCLIncrementsStatForExplicitOnFailure() + { + AbstractWriteResponseHandler awr = createWriteResponseHandler(ConsistencyLevel.LOCAL_QUORUM, ConsistencyLevel.EACH_QUORUM); + + long startingCountForWriteFailedIdealCL = ks.metric.writeFailedIdealCL.getCount(); + long startingCountForIdealCLWriteLatency = ks.metric.idealCLWriteLatency.totalLatency.getCount(); + + + //Succeed in local DC + awr.onResponse(createDummyMessage(0)); + awr.onResponse(createDummyMessage(1)); + awr.onResponse(createDummyMessage(2)); + + + //Fail in remote DC + awr.onFailure(targets.get(3).endpoint(), RequestFailureReason.TIMEOUT); + awr.onFailure(targets.get(4).endpoint(), RequestFailureReason.TIMEOUT); + awr.onResponse(createDummyMessage(5)); + + assertEquals(startingCountForWriteFailedIdealCL + 1, ks.metric.writeFailedIdealCL.getCount()); + assertEquals(startingCountForIdealCLWriteLatency, ks.metric.idealCLWriteLatency.totalLatency.getCount()); + } + /** * Validate that failing to achieve ideal CL doesn't increase the failure counter when not meeting CL * @throws Throwable @@ -258,6 +289,30 @@ public class WriteResponseHandlerTest assertEquals(startingCount, ks.metric.writeFailedIdealCL.getCount()); } + @Test + public void failedIdealCLDoesNotIncrementsStatOnExplicitQueryFailure() + { + AbstractWriteResponseHandler awr = createWriteResponseHandler(ConsistencyLevel.LOCAL_QUORUM, ConsistencyLevel.EACH_QUORUM); + + long startingCountForWriteFailedIdealCL = ks.metric.writeFailedIdealCL.getCount(); + long startingCountForIdealCLWriteLatency = ks.metric.idealCLWriteLatency.totalLatency.getCount(); + + + //Fail in local DC + awr.onFailure(targets.get(0).endpoint(), RequestFailureReason.TIMEOUT); + awr.onFailure(targets.get(1).endpoint(), RequestFailureReason.TIMEOUT); + awr.onResponse(createDummyMessage(2)); + + + //Fail in remote DC + awr.onFailure(targets.get(3).endpoint(), RequestFailureReason.TIMEOUT); + awr.onFailure(targets.get(4).endpoint(), RequestFailureReason.TIMEOUT); + awr.onResponse(createDummyMessage(5)); + + assertEquals(startingCountForWriteFailedIdealCL, ks.metric.writeFailedIdealCL.getCount()); + assertEquals(startingCountForIdealCLWriteLatency, ks.metric.idealCLWriteLatency.totalLatency.getCount()); + } + private static AbstractWriteResponseHandler createWriteResponseHandler(ConsistencyLevel cl, ConsistencyLevel ideal) {