[
https://issues.apache.org/jira/browse/CASSANDRA-1072?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12971712#action_12971712
]
Sylvain Lebresne commented on CASSANDRA-1072:
---------------------------------------------
Thanks for the rebasing. A few comments/remarks:
# add and batch_add should throw an InvalidRequestException if CL != CL.ONE.
Right now this is checked by an assert in StorageProxy which is far too
late.
# Why doesn't the code really stuff the partitionedCounter inside the column
value ? Keeping the value and the partionedCounter aside seems wasteful. It
wastes space (the information is stored twice) and it wastes cpu (the
total is recomputed each time the context is modified, in particular after
each call to reconcile, while in practice is would be enough to compute it
just before returning the final value to the client). I admit the cpu part
is not so clear as if a counter doesn't change, we avoid recomputing the
value on each read by keeping it aside. I still think this would be a win
to no keep the pre-computed value. And anyway, I find this juggling
between the value and the partionedCounter adds clutter and is error-prone.
# That's not really a remark on the patch itself, but the patch includes the
fix for CASSANDRA-1830, so I think we'd better commit the #1830 first and
rebase this afterwards.
A few other more minor remarks:
# Renaming repair_on_write to replicate_on_write would make me happy, in
that I do hope we can later improve this mechanism so that it helps
establish the usual consistency guarantees. In which case I believe
replicate_on_write will be a more precise and less confusing
name.</nitpicking>
# About the repair on write, I would actually be in favor of renaming it
further in replicate_counter_on_write (or repair_counter_on_write :))
and/or disallowing it for non counter CFs. Even though there is nothing
intrinsically wrong with doing repair_on_write on non counter CFs, I think
it would be a disservice to users to let them think it could be useful
(unless it does have a use for non counter CFs, but I really don't see any).
# A few lines in ColumnSerializer do not follow the coding style.
Arguably those are either trivially fixable and/or more a matter of opinion
than anything. The exception being maybe the point on getting rid of keeping
both value and partionedCounter. I do believe it would be a nice change but I
admit this is not crucial and I would be OK with committing as is and keep
this as a future refactoring in the interest of having things move forward
(note however that changing this will break the on disk format, but I think we
agreed that we'll probably break it anyway to introduce uuids). Anyway, I
would still love to get your opinions on the issue :)
> Increment counters
> ------------------
>
> Key: CASSANDRA-1072
> URL: https://issues.apache.org/jira/browse/CASSANDRA-1072
> Project: Cassandra
> Issue Type: Sub-task
> Components: Core
> Reporter: Johan Oskarsson
> Assignee: Kelvin Kakugawa
> Attachments: CASSANDRA-1072.121310.patch, CASSANDRA-1072.patch,
> increment_test.py, Partitionedcountersdesigndoc.pdf
>
>
> Break out the increment counters out of CASSANDRA-580. Classes are shared
> between the two features but without the plain version vector code the
> changeset becomes smaller and more manageable.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
