You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "David Capwell (Jira)" <ji...@apache.org> on 2020/04/23 22:54:00 UTC
[jira] [Comment Edited] (CASSANDRA-15718) Improve BatchMetricsTest
[ https://issues.apache.org/jira/browse/CASSANDRA-15718?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17090990#comment-17090990 ]
David Capwell edited comment on CASSANDRA-15718 at 4/23/20, 10:53 PM:
----------------------------------------------------------------------
> Can we remove ".withExamples(10)", should rely on the default.
you bumped to 25, so feel thats a fair compromise; thanks =)
Overall the patch LGTM, there is a very small change to the test to simplify the min/max logic; I attached a patch to your branch to do that (also, the tests are flaky caused by timeouts, so bump the timeout value). The basic idea is that getBucketingForValue doesn't return the bucket value, but the range of values. Since we always apply a fixed partition size to the test min is always max_bucket-1 (exception when bucket == 0), so by making the function return the range, we can do a simple equals check on the pair to assert min/max at the same time.
was (Author: dcapwell):
> Can we remove ".withExamples(10)", should rely on the default.
you bumped to 25, so feel thats a fair compromise; thanks =)
Overall the patch LGTM, there is a very small change to the test to simplify the min/max logic; I attached a patch to your branch to do that (also, the tests are flaky caused by timeouts, so bump the timeout value).
> Improve BatchMetricsTest
> -------------------------
>
> Key: CASSANDRA-15718
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15718
> Project: Cassandra
> Issue Type: Improvement
> Components: Test/unit
> Reporter: Stephen Mallette
> Assignee: Stephen Mallette
> Priority: Normal
> Attachments: CASSANDRA-15582-test-cleanup.patch
>
>
> As noted in CASSANDRA-15582 {{BatchMetricsTest}} should test {{BatchStatement.Type.COUNTER}} to cover all the {{BatchMetrics}}. Some changes were introduced to make this improvement at:
> https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics
> and the following suggestions were made in review (in addition to the suggestion that a separate JIRA be created for this change) by [~dcapwell]:
> {quote}
> * I like the usage of BatchStatement.Type for the tests
> * honestly feel quick theories is better than random, but glad you added the seed to all asserts =). Would still be better as a quick theories test since you basically wrote a property anyways!
> * https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics#diff-8948cec1f9d33f10b15c38de80141548R131 feel you should rename to expectedPartitionsPerLoggedBatch {Count,Logged,Unlogged}
> * . pre is what the value is, post is what the value is expected to be (rather than what it is).
> *
> * https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics#diff-8948cec1f9d33f10b15c38de80141548R150 this doesn't look correct. the batch has distinctPartitions mutations, so shouldn't max reflect that? I ran the current test in a debugger and see that that is the case (aka current test is wrong).
> most of the comments are nit picks, but the last one looks like a test bug to me
> {quote}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org