cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ariel Weisberg (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-9932) Make all partitions btree backed
Date Wed, 05 Aug 2015 20:39:04 GMT


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:
>             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

View raw message