[
https://issues.apache.org/jira/browse/CASSANDRA-6506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13940608#comment-13940608
]
Sylvain Lebresne commented on CASSANDRA-6506:
---------------------------------------------
To me, the main interest of doing this is to as much of special casing of
counters at the storage engine layer as possible, to basically make counters
just a bunch of normal cells. In that regard, I'm not convinced it's worth
having special cellname implementations: the 3 methods added could easily be
static methods of say CounterColumns, thus removing a bunch of code, and making
it clear counters are just an encoding on top of the storage engine.
I'm also uncomfortable with originalType() (and enableCounters() to some
extend). It feels pretty error prone as you need to think hard about whether
you you call that first before calling some other method or not. It feels to me
that again, we shouldn't need anything special with CellNameType for counters:
thrift/CQL should convert things to/from the "internal" format, but as far as
CellName/CellNameType are concerned, we shouldn't need anything special casing
at all (I do understand that we probably need to start storing whether the
table is dense or not in the system table for thrift to convert things
properly, but it's high time we do that anyway).
In fact, I'm starting to think that the best way to do this would be to
internally deal with counters as (CQL) maps of shard id -> count. This would
allow reuse of the existing as often as possible (we wouldn't need modification
of CQL3RowOfSparse for instance). One obstacle to that is that collections only
work with true CQL3 table, and we need to deal with thrift CF of counters, but
honestly, I think I'd be fine encoding those as true CQL3 tables under the hood
having just one CQL column (that could have an empty name). With compression
and since counters table will be composite under the hood anyway, I'm not sure
the overhead would be noticeable and it would definitively simplify matters.
Also, tbh, I'm starting to wonder if 2.1 is such a reasonable target for this
at this point. Especially given we can't get rid of CounterCell right now (I
think we should seriously consider getting rid of local/remote shard for 3.0)
this feels like a lot of changes to push at the last minute, and it doesn't
felt like it brings so much to the table that it can't wait 3.0. Especially
since in 3.0 it might be achievable to get rid of CounterCell once and for all
(and we wouldn't have to care about CQL2).
Outside of those general comments, a few minor remarks/nits gathered from
reading the patch:
* isSameCQL3RowAs should use the comparator, not equals().
* I believe disallowing the drop of columns for CQL3 counter tables is a
regression. Not sure why the patch does it tbh.
* Nit: I'd remove the 'clock' arg to CounterUpdateCell.create() and hardcode it
to 1 inside the method to avoid misuse.
* Nit: I was liking the inital comment of CounterUpdateCell.reconcile, I'd be
happy keeping it.
> counters++ split counter context shards into separate cells
> -----------------------------------------------------------
>
> Key: CASSANDRA-6506
> URL: https://issues.apache.org/jira/browse/CASSANDRA-6506
> Project: Cassandra
> Issue Type: Improvement
> Reporter: Aleksey Yeschenko
> Assignee: Aleksey Yeschenko
> Fix For: 2.1 beta2
>
>
> This change is related to, but somewhat orthogonal to CASSANDRA-6504.
> Currently all the shard tuples for a given counter cell are packed, in sorted
> order, in one binary blob. Thus reconciling N counter cells requires
> allocating a new byte buffer capable of holding the union of the two
> context's shards N-1 times.
> For writes, in post CASSANDRA-6504 world, it also means reading more data
> than we have to (the complete context, when all we need is the local node's
> global shard).
> Splitting the context into separate cells, one cell per shard, will help to
> improve this. We did a similar thing with super columns for CASSANDRA-3237.
> Incidentally, doing this split is now possible thanks to CASSANDRA-3237.
> Doing this would also simplify counter reconciliation logic. Getting rid of
> old contexts altogether can be done trivially with upgradesstables.
> In fact, we should be able to put the logical clock into the cell's
> timestamp, and use regular Cell-s and regular Cell reconcile() logic for the
> shards, especially once we get rid of the local/remote shards some time in
> the future (until then we still have to differentiate between
> global/remote/local shards and their priority rules).
--
This message was sent by Atlassian JIRA
(v6.2#6252)
