[
https://issues.apache.org/jira/browse/CASSANDRA-15718?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17090727#comment-17090727
]
Stephen Mallette commented on CASSANDRA-15718:
----------------------------------------------
> Should call snapshot once, else you do more memory copies than needed.
fixed
> Can we remove ".withExamples(10)", should rely on the default.
I can remove it, but it will drastically increase the test time from 3 seconds
to almost 3 minutes on my system. Is this test validating behavior and ranges
worth 3 minutes? I wasn't sure, which is why i figured hitting a subset of
examples would be a sufficient improvement over the more static approach that
existed before I began my changes. Perhaps I could change other parameters that
would produce less tests instead (e.g. reduce partition counts tested instead)?
I've left that as is for now, but happy to change it to whatever makes sense.
Just let me know.
> this doesn't look stable it assumes the very first test case has 1 partition,
> if the first test case has 2 or more partitions then it should fail (since
> the value is now 2 [1]).
min/max has been tricky but I ran with your approach to include that test
method you suggested - if you have a better name for that method I'm fine to
take suggestions. it deals with the max pretty nicely. min seemed to require a
bit more thought but basically it appeared to require that i retrieve the
bucketing prior to the max and then assert a {{<=}} sort of range as existed
before. I'm not quite sure if I'm missing something obvious in how to assert
the exact min expectation, but my debug sessions never quite got to the bottom
of that. :(
> Would be good to call
> "org.apache.cassandra.metrics.DecayingEstimatedHistogramReservoir#clear" so
> each test doesn't see the results of the previous tests.
I added that call to {{clear()}} and I think that makes for a better test. I'd
thought about that but didn't realize the approach to do so was so
straightforward. Thanks for pointing me at that.
Looking forward to hearing what folks think of this latest revision.
> 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
