cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Aleksey Yeschenko (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-9705) Simplify some of 8099's concrete implementations
Date Tue, 21 Jul 2015 18:01:07 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-9705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14635504#comment-14635504
] 

Aleksey Yeschenko commented on CASSANDRA-9705:
----------------------------------------------

Fill disclosure:  the patch set is very sizable, so this is not a complete thorough review
- merely an overeview answering the question 'can we merge this into trunk as is, and finish
the review afterwards?'.

Broken:
1. {{ClusteringComponent.compareComponent()}} is broken for v1 == v2 == null (the first ternary
is wrong). We need tests for this.

Looks fishy:
2. {{RangeTombstoneBoundaryMarker}} {{createCorrespondingCloseMarker()}} and {{createCorrespondingOpenMarker()}}
have swapped their behavior in this patch set. I'm assuming that this was intentional, but
if so, it at least warrants a comment

Unused fields and method arguments that are confusing (some not new to this patch set). Most
of these, if not all, are benign, but some wasted some time to figure out:
3. {{UnfilteredDeserializer.OldFormatDeserializer.nextFlags}} is unused
4. {{MemtableAllocator.SubAllocator}} fields {{owns}} and {{reclaiming}} are unused
5. {{BitTableWriter.StatsCollector.collectStatsOn()}} is unused (but turns out to be benign)
6. {{SerializationHelper.endOfComplexColumn()}} method has an unused {{column}} argument

Nits:
7. {{PartitionUpdate.CounterMark}} class should be static

Nomenclature is arguable, and there is plenty of minor nits that I won't list here (but would
urge Sylvain to go through each file in IDEA and pay attention to the inspections).

Once cassci is happy (which is to say, once it shows that these changes don't break anything
new), I'm fine with committing it to trunk. Overall it's a welcome simplification that should
fix and make a lot of things easier (including Tyler's backward compat work and Sam's 2i work).

> Simplify some of 8099's concrete implementations
> ------------------------------------------------
>
>                 Key: CASSANDRA-9705
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-9705
>             Project: Cassandra
>          Issue Type: Sub-task
>            Reporter: Sylvain Lebresne
>            Assignee: Sylvain Lebresne
>             Fix For: 3.0 beta 1
>
>
> As mentioned in the ticket comments, some of the concrete implementations (for Cell,
Row, Clustering, PartitionUpdate, ...) of the initial patch for CASSANDRA-8099 are more complex
than they should be (the use of flyweight is typically probably ill-fitted), which probably
has performance consequence. This ticket is to track the refactoring/simplifying those implementation
(mainly by removing the use of flyweights and simplifying accordingly).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message