[jira] [Commented] (CASSANDRA-10059) Test Coverage and related bug-fixes for AbstractBTreePartition and hierarchy

["Sylvain Lebresne (JIRA)" <ji...@apache.org>]

    [ 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)