cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <>
Subject [jira] Commented: (CASSANDRA-1072) Increment counters
Date Wed, 23 Jun 2010 16:57:51 GMT


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 ? 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
    if (clock.size() == DBConstants.intSize_ + IncrementCounterContext.HEADER_LENGTH)
    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

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

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message