[
https://issues.apache.org/jira/browse/CASSANDRA-1072?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12881747#action_12881747
]
Sylvain Lebresne commented on CASSANDRA-1072:
---------------------------------------------
A bunch of remarks/comments/questions on the patch.
- I think the replication logic should be fixed before the patch could get in.
About that, for consistency >= ONE, wouldn't it work to pick a replica, send
it the update, wait for the ack and then, send it to all other nodes
(then blocking for whatever number of node we need to achieve consistency) ?
Please tell me if I'm completely off here.
- I think that if client insert negative value for increment counters, bad
stuffs will happened. The code should check for it (in Thrift validation
most probably). It should also check that the value is a long.
- IncrementCounterReconciler.updateDeleteTimestamp() sets the delete
timestamp of the live column to the max of the timestamps of the live and
deleted columns. Shouldn't it set the max of the 'delete' timestamps ?
- I'm not convinced by the thrift API for creating increment counter. Calling
Clock() doesn't at all make it explicit that we want increment counter. More
generally, will we ever want to expose the binary context to the client ?
- There is a bunch of code duplication for AESCompactionIterator, even though
it is just an AntiCompactionIterator that cleans contexts. Would it be better
to merge those two by adding a flag saying if the context must be cleaned or
not ? Even if we don't, the switch in doAESCompaction could be removed.
- In ReadResponseResolver.resolveSuperset(), the cf.cloneMe() is changed in
cf.cloneMeShallow(). Out of curiosity, is there a reason for it other than
for efficiency and are we sure it is safe to do ?
- What about increment counters when we have
https://issues.apache.org/jira/browse/CASSANDRA-1210 ? I don't know what was
planned for the latter one but if it requires few changes with respect to
the increment only counters (don't a map of id -> (count, version) instead
of a map of id -> counts suffices ?), maybe we should got with #1210 right
away ?
Other more minor comments:
- In IncrementCounterReconciler.reconcile(), I think it would be more clear to
replace
if (clock.size() == DBConstants.intSize_ +
IncrementCounterContext.HEADER_LENGTH)
by
if (clock.context.length == IncrementCounterContext.HEADER_LENGTH)
since it's weird to compare the serialized size here.
Moreover, if we check upfront that clients cannot put bad values in counter
updates, the checks that follows are unnecessary.
- In IncrementCountContext.merge(), when computing the sum for a given id, the
code checks if id is in contextsMap and if not do a get and a put. It should
be a bit more efficient to start by doing a put of (id, count), and since a
put returns the old value, to correct by a second put if needed.
- Column.comparePriority() is now dead code.
- IncrementCounterContext.diff() doesn't respect the code style for a few
braces.
- The constructor with no argument of TimestampClock should be removed.
- Last, and probably least, but I'll still mention it. The code uses at
multiple times the pattern:
someloop {
if (somecondition) {
doSomething;
continue;
}
doSomethingElse;
}
I'm not fond of it as I find it more difficult to follow what gets executed
than with a good old fashioned 'else'. But maybe that's me.
> 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-2.patch, CASSANDRA-1072.patch
>
>
> 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.
