[
https://issues.apache.org/jira/browse/CASSANDRA-15718?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17083351#comment-17083351
]
Stephen Mallette commented on CASSANDRA-15718:
----------------------------------------------
I've pushed some changes to my branch (link in description) addressing the
points that were made in the initial review. I converted to quick theories
rather than my custom use of {{Random}}. I wasn't quite sure how to tune the
parameters to the test exactly as running these tests over larger parameter
spaces adds to the total run time of the test. I ended up settling on
parameters that routinely ran in less than one to two seconds total for the
entire test class. If someone thinks there is value in tuning quick theories to
run more examples with wider parameters it's easy enough to change. For my own
purposes/testing, I've run these tests with significantly wider testing
parameters and they pass without issues.
As for this point:
> the batch has distinctPartitions mutations, so shouldn't max reflect that?
I spent a fair amount of time trying to understand exactly what was happening
there and I could still have it wrong but I think that the semantics of {{<=}}
for the assertion might have been correct, though the numbers I asserted in my
test were originally wrong. Given the underlying use of
{{DecayingEstimatedHistogramReservoir}} for the batch metrics it seems that
it's possible for the actual value recorded in the reservoir to be different
from the results returned from {{getMin()}} and {[getMax()}} so even though we
know the {{distinctPartitions}} I'm not sure that we know exactly what the
returned value might be. Of course, it's entirely possible that I've completely
misunderstood something here.
Looking forward to the next round of review on this and if some aspect of this
can be further improved.
> 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
>
> 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-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
