You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/03/17 22:22:47 UTC
[GitHub] [cassandra] bbotella opened a new pull request #1509: Add proper hitogram checkup to fix flaky failThresholdSinglePartition
bbotella opened a new pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509
Add proper hitogram checkup to fix flaky failThresholdSinglePartition test
patch by Bernardo Botella Corbi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r832673314
##########
File path: test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java
##########
@@ -283,15 +295,26 @@ public boolean matches(InetAddress value)
}
assertHistogramUpdated();
assertWarnAborts(0, 2, 1);
+ }
+
+ public void failThresholdDisabled(String cql) throws UnknownHostException
+ {
+ ICoordinator node = CLUSTER.coordinator(1);
+ for (int i = 0; i < failThresholdRowCount(); i++)
+ node.execute("INSERT INTO " + KEYSPACE + ".tbl (pk, ck, v) VALUES (1, ?, ?)", ConsistencyLevel.ALL, i + 1, bytes(512));
+
+ if (shouldFlush())
+ CLUSTER.stream().forEach(i -> i.flush(KEYSPACE));
// query should no longer fail
enable(false);
+ checkpointHistogram();
Review comment:
this has to go after the query
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] yifan-c commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
yifan-c commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r837735224
##########
File path: CHANGES.txt
##########
@@ -1,3 +1,5 @@
+Add proper hitogram checkup to fix flaky failThresholdSinglePartition test
Review comment:
> not proper format, I would personally remove and leave to committer
Agree.
And... this patch is to fix test only. So we will not add an entry in the `CHANGES.txt`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] sathyakplm commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
sathyakplm commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r839495191
##########
File path: test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java
##########
@@ -283,15 +295,26 @@ public boolean matches(InetAddress value)
}
assertHistogramUpdated();
assertWarnAborts(0, 2, 1);
+ }
+
+ public void failThresholdDisabled(String cql) throws UnknownHostException
+ {
+ ICoordinator node = CLUSTER.coordinator(1);
+ for (int i = 0; i < failThresholdRowCount(); i++)
+ node.execute("INSERT INTO " + KEYSPACE + ".tbl (pk, ck, v) VALUES (1, ?, ?)", ConsistencyLevel.ALL, i + 1, bytes(512));
+
+ if (shouldFlush())
+ CLUSTER.stream().forEach(i -> i.flush(KEYSPACE));
// query should no longer fail
enable(false);
+ checkpointHistogram();
SimpleQueryResult result = node.executeWithResult(cql, ConsistencyLevel.ALL);
assertThat(result.warnings()).isEmpty();
assertHistogramNotUpdated();
Review comment:
One minor query. In Line 313, we are asserting that the warnings is empty. Isn't that sufficient? Do we still need to check if the histogram hasn't updated? Isn't it redundant?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] bbotella commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
bbotella commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r839729728
##########
File path: test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java
##########
@@ -283,15 +295,26 @@ public boolean matches(InetAddress value)
}
assertHistogramUpdated();
assertWarnAborts(0, 2, 1);
+ }
+
+ public void failThresholdDisabled(String cql) throws UnknownHostException
+ {
+ ICoordinator node = CLUSTER.coordinator(1);
+ for (int i = 0; i < failThresholdRowCount(); i++)
+ node.execute("INSERT INTO " + KEYSPACE + ".tbl (pk, ck, v) VALUES (1, ?, ?)", ConsistencyLevel.ALL, i + 1, bytes(512));
+
+ if (shouldFlush())
+ CLUSTER.stream().forEach(i -> i.flush(KEYSPACE));
// query should no longer fail
enable(false);
+ checkpointHistogram();
SimpleQueryResult result = node.executeWithResult(cql, ConsistencyLevel.ALL);
assertThat(result.warnings()).isEmpty();
assertHistogramNotUpdated();
Review comment:
I didn't want to change current behavior. As you can see in the original test:
https://github.com/apache/cassandra/blob/trunk/test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java#L206-L207
It was asserting both things already. Same goes for the failThreshold method:
https://github.com/apache/cassandra/blob/trunk/test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java#L290-L291
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r837731107
##########
File path: CHANGES.txt
##########
@@ -1,3 +1,5 @@
+Add proper hitogram checkup to fix flaky failThresholdSinglePartition test
Review comment:
not proper format, I would personally remove and leave to committer
If you want it correctly (which always conflicts on merge so committer has to fix... hence why I always exclude... but thats me), you would put this under `4.1` (if its going in the 4.1 release), put it at the top and add ` * ` prefix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] dcapwell commented on a change in pull request #1509: CASSANDRA-17217 Add proper histogram checkup to fix flaky failThresholdSinglePartition
Posted by GitBox <gi...@apache.org>.
dcapwell commented on a change in pull request #1509:
URL: https://github.com/apache/cassandra/pull/1509#discussion_r837733133
##########
File path: test/distributed/org/apache/cassandra/distributed/test/trackwarnings/AbstractClientSizeWarning.java
##########
@@ -283,15 +295,26 @@ public boolean matches(InetAddress value)
}
assertHistogramUpdated();
assertWarnAborts(0, 2, 1);
+ }
+
+ public void failThresholdDisabled(String cql) throws UnknownHostException
+ {
+ ICoordinator node = CLUSTER.coordinator(1);
+ for (int i = 0; i < failThresholdRowCount(); i++)
+ node.execute("INSERT INTO " + KEYSPACE + ".tbl (pk, ck, v) VALUES (1, ?, ?)", ConsistencyLevel.ALL, i + 1, bytes(512));
+
+ if (shouldFlush())
+ CLUSTER.stream().forEach(i -> i.flush(KEYSPACE));
// query should no longer fail
enable(false);
SimpleQueryResult result = node.executeWithResult(cql, ConsistencyLevel.ALL);
+ checkpointHistogram();
Review comment:
this has to be before query, as we are checking if the query triggered the condition or not
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org