[
https://issues.apache.org/jira/browse/CASSANDRA-10059?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15035782#comment-15035782
]
Sylvain Lebresne commented on CASSANDRA-10059:
----------------------------------------------
Slightly late to the review, sorry. Test is convincing in general and fixes
looks good but a few small remarks on the test:
* In the {{test(Supplier ...)}} method, the lack of both braces and indentation
makes this hard to read.
* In the main {{testIter()}} method, there is a sub-part about {{// filtered}},
but while this filter on the expected input, it doesn't filter the actual
iterator. The test still pass because the row generated by the test never have
(cell) tombstones, so there is nothing filtered, but it's still kind of wrong.
We can randomly generate some tombstone when generating rows and use
{{UnfilteredRowIterators.filter()}} on the iterator from the partition, but
that's more a test of {{UnfilteredRowIterators}} that of the partition
implementation itself, so I'm fine if you prefer just removing that case here.
* Nitpicking here: one assumption of a partition is that it should not contain
data that it deletes itself, which in practice mean that any cell covered by a
marker should have a timestamp greater than marker deletion time. The test
mostly respect that thanks to the use of the current time for row timestamp but
a random number <= {{KEY_RANGE}} for range markers, but that feels a bit like a
luck as is. Maybe using {{KEY_RANGE + 1}} as timestamp for the row cells with
a short comment would be more explicit.
Could you also make sure the branch is rebased and CI are ran on it.
> Test Coverage and related bug-fixes for AbstractBTreePartition and hierarchy
> ----------------------------------------------------------------------------
>
> Key: CASSANDRA-10059
> URL: https://issues.apache.org/jira/browse/CASSANDRA-10059
> Project: Cassandra
> Issue Type: Test
> Components: Local Write-Read Paths
> Reporter: Benedict
> Assignee: Branimir Lambov
> Fix For: 3.0.1, 3.1
>
>
> Follow up to CASSANDRA-9932. The test coverage for AbstractBTreePartition and
> its hierarchy is entirely indirect. That is not to say it is not covered, but
> we may have some unexplored behaviour. Coverage for BTree is also missing
> around a couple of edges, and the gaps should be filled in.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
