Details
-
Improvement
-
Status: Resolved
-
Normal
-
Resolution: Fixed
-
None
-
Quality Assurance
-
Low Hanging Fruit
-
All
-
None
-
Description
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:
- 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
Unknown macro: {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
Attachments
Attachments
Issue Links
- is a child of
-
CASSANDRA-15582 4.0 quality testing: metrics
-
- In Progress
-