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