[ https://issues.apache.org/jira/browse/CASSANDRA-9932?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14658873#comment-14658873 ]
Ariel Weisberg commented on CASSANDRA-9932: ------------------------------------------- This looks really nice especially how it removes duplicated code. At a high level I have nothing to complain about. At a low level it's pretty hard to have confidence just inspecting the code I just want to focus on how convincing the tests are. I will do coverage for the utests now, and continue reviewing after. Code coverage for utests, and dtests separately and together would be helpful in reviewing this. I could generate that myself if I knew the magic incantation for code coverage and dtests. This change to a large extent looks like a refactor and not new code so I am trying not to wade too deep into the existing code although as you will see I fail at that in my review. CachedPartition still refers to ArrayBackedPartition in comments. Worth a pass to clean up comments pointing to the removed classes. With default methods it really seems to me that we always want @Override annotations. When someone adds a default they can be careful and add the @Override, but when someone is implementing an interface to they have go and check which ones have defaults and just add it for those or use them all the time? For me the refactor pain from missing annotations heavily outweighs the extra typing/text. AbstractBTreePartition.Holder.with appears to be unused. I ran code coverage for some tests and there seems to be untested stuff in BTree such as transformAndFilter. reverse() which is new for this patch also has no coverage. I didn't check coverage from other tests. I don't see unit tests for the various partition types in isolation AtomicBTreePartition from PartitionTest doesn't cover stuff like waste tracking or pessimistic locking. Is that coverage supposed to come indirectly via Memtable tests? AbstractBTreePartition also has stuff that isn't tested. > Make all partitions btree backed > -------------------------------- > > Key: CASSANDRA-9932 > URL: https://issues.apache.org/jira/browse/CASSANDRA-9932 > Project: Cassandra > Issue Type: Improvement > Components: Core > Reporter: Benedict > Assignee: Benedict > Fix For: 3.0.0 rc1 > > > Following on from the other btree related refactors, this patch makes all > partition (and partition-like) objects backed by the same basic structure: > {{AbstractBTreePartition}}. With two main offshoots: > {{ImmutableBTreePartition}} and {{AtomicBTreePartition}} > The main upshot is a 30% net code reduction, meaning better exercise of btree > code paths and fewer new code paths to go wrong. A secondary upshort is that, > by funnelling all our comparisons through a btree, there is a higher > likelihood of icache occupancy and we have only one area to focus delivery of > improvements for their enjoyment by all. -- This message was sent by Atlassian JIRA (v6.3.4#6332)