Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 0.7 beta 2
    • Component/s: API
    • Labels:
      None

      Description

      Clock was introduced back when we insufficiently understood the limitations of CASSANDRA-580. It turns out to not be necessary. This ticket removes it from the Thrift API to get ready for 0.7.0; removing it from the rest of the internals can follow.

      1. 1501-v3.txt
        227 kB
        Jonathan Ellis
      2. 1501-v2.txt
        152 kB
        Jonathan Ellis
      3. 1501.txt
        125 kB
        Jonathan Ellis

        Activity

        Hide
        Jonathan Ellis added a comment -

        committed.

        (someone must be using an old version of the thrift compiler, I rebuilt from r959516 which is the version we're supposed to be using and the hashcode implementations are as you see here.)

        Show
        Jonathan Ellis added a comment - committed. (someone must be using an old version of the thrift compiler, I rebuilt from r959516 which is the version we're supposed to be using and the hashcode implementations are as you see here.)
        Hide
        Sylvain Lebresne added a comment -

        +1

        As a very minor detail, the patch includes changes to the thrift generated files that are unrelated to
        the ticket. Seems more like an update of the thrift compiler (basically, the hashCodes for the generated
        code aren't dumb anymore). We may or may not want that to go with this patch.

        Show
        Sylvain Lebresne added a comment - +1 As a very minor detail, the patch includes changes to the thrift generated files that are unrelated to the ticket. Seems more like an update of the thrift compiler (basically, the hashCodes for the generated code aren't dumb anymore). We may or may not want that to go with this patch.
        Hide
        Jonathan Ellis added a comment -

        v3 rebases and changes clock to timestamp in Deletion and remove.

        Show
        Jonathan Ellis added a comment - v3 rebases and changes clock to timestamp in Deletion and remove.
        Hide
        Sylvain Lebresne added a comment -

        The patch needs rebasing (because of the min/max compaction thresholds I believe).
        Also, minor detail, in cassandra.thrift, in remove() and in the Deletion structure, the
        argument is still called clock.

        The rest looks fine, but I wasn't able to apply cleanly.

        Show
        Sylvain Lebresne added a comment - The patch needs rebasing (because of the min/max compaction thresholds I believe). Also, minor detail, in cassandra.thrift, in remove() and in the Deletion structure, the argument is still called clock. The rest looks fine, but I wasn't able to apply cleanly.
        Hide
        Jonathan Ellis added a comment -

        * In test_thrift_server.py, there is a Clock remaining in test_cf_recreate().

        fixed

        * As remarked by Johan, in the thift interface, the timestamp is still called clock

        updated to `timestamp`

        * Cli client doesn't remove fully reconciler and clock_type

        fixed

        * the avro API.

        removed Clock from Avro as well

        * the fact that DatabaseDescriptor.readTablesFromYaml() (and RawColumnFamily) still tries to read a reconciler and clock_type.

        updated

        Show
        Jonathan Ellis added a comment - * In test_thrift_server.py, there is a Clock remaining in test_cf_recreate(). fixed * As remarked by Johan, in the thift interface, the timestamp is still called clock updated to `timestamp` * Cli client doesn't remove fully reconciler and clock_type fixed * the avro API. removed Clock from Avro as well * the fact that DatabaseDescriptor.readTablesFromYaml() (and RawColumnFamily) still tries to read a reconciler and clock_type. updated
        Hide
        Jonathan Ellis added a comment -

        it's not just performance; as Sylvain says, if we can fit it in a different Column type like the ttl code, it's just cleaner design (and affects the rest of the code a lot less).

        i'm a huge non-fan of IClock and AbstractReconciler after having to genuflect to them for a couple months now.

        Show
        Jonathan Ellis added a comment - it's not just performance; as Sylvain says, if we can fit it in a different Column type like the ttl code, it's just cleaner design (and affects the rest of the code a lot less). i'm a huge non-fan of IClock and AbstractReconciler after having to genuflect to them for a couple months now.
        Hide
        Johan Oskarsson added a comment -

        The discussion seems to boil down to performance concerns of having TimestampClock wrap the timestamp long variable in the standard write path. I'm not convinced the overhead is big enough to be an issue. Would it not be similar to Long vs long for example? Have you measured it on a running Cassandra instance with a benchmark or profiler?

        Refactoring CASSANDRA-1072 to work without the internal clock code (IClock + impl, ClockType, Reconciler etc) is a massive undertaking and at this stage not even guaranteed to be feasible in the manner suggested with, I suspect, minimal benefits.

        Show
        Johan Oskarsson added a comment - The discussion seems to boil down to performance concerns of having TimestampClock wrap the timestamp long variable in the standard write path. I'm not convinced the overhead is big enough to be an issue. Would it not be similar to Long vs long for example? Have you measured it on a running Cassandra instance with a benchmark or profiler? Refactoring CASSANDRA-1072 to work without the internal clock code (IClock + impl, ClockType, Reconciler etc) is a massive undertaking and at this stage not even guaranteed to be feasible in the manner suggested with, I suspect, minimal benefits.
        Hide
        Sylvain Lebresne added a comment -

        On the patch itself, and as far as removing clocks from thrift api is
        concerned, few minor comments:

        • In test_thrift_server.py, there is a Clock remaining in test_cf_recreate().
          I also get 3 errors in the avro system tests, but it seems unrelated.
        • As remarked by Johan, in the thift interface, the timestamp is still called
          clock. It's not a bad name per se, but renaming it back to timestamp would
          ease migration for those coming from 0.6.
        • Cli client doesn't remove fully reconciler and clock_type (in the
          AddColumnFamilyArgument enum and at line 261, in the add column family
          documentation). It's not completely necessary, but as is the
          assert at the end of the switch in executeAddColumnFamily() is wrong.

        The patch does not remove the clocks from all of what I could call 'the
        cassandra exported api'. I'm mostly thinking of:

        • the avro API.
        • the fact that DatabaseDescriptor.readTablesFromYaml() (and
          RawColumnFamily) still tries to read a reconciler and clock_type.
          The ticket's name makes it clear that it is about thrift, and if not done
          before, that will have to be tackled in #1502, but as it's not strictly
          speaking 'internals', I thought I'd mentionned it (in particular, whatever we
          decide for CASSANDRA-1502, those will have to be done).

        I agree that for CASSANDRA-1072, we need some way to distinguish between cfs with
        counters and other cfs. But if we're just talking counters, and as far as the
        thrift api is concerned, it will be enough to have a 'Counter' cf type (and
        a 'SuperCounter' one). Or, alternatively, we could user the validateWith
        option to distinguish counter cfs. So I see no reason why we shouldn't remove
        clock_type and reconciler in this patch. Modulo the (minor) comments above,
        I'm +1 on this.

        Removing the clock interface from the internals is another matter, but I'll
        comment on the CASSANDRA-1502 ticket.

        Show
        Sylvain Lebresne added a comment - On the patch itself, and as far as removing clocks from thrift api is concerned, few minor comments: In test_thrift_server.py, there is a Clock remaining in test_cf_recreate(). I also get 3 errors in the avro system tests, but it seems unrelated. As remarked by Johan, in the thift interface, the timestamp is still called clock. It's not a bad name per se, but renaming it back to timestamp would ease migration for those coming from 0.6. Cli client doesn't remove fully reconciler and clock_type (in the AddColumnFamilyArgument enum and at line 261, in the add column family documentation). It's not completely necessary, but as is the assert at the end of the switch in executeAddColumnFamily() is wrong. The patch does not remove the clocks from all of what I could call 'the cassandra exported api'. I'm mostly thinking of: the avro API. the fact that DatabaseDescriptor.readTablesFromYaml() (and RawColumnFamily) still tries to read a reconciler and clock_type. The ticket's name makes it clear that it is about thrift, and if not done before, that will have to be tackled in #1502, but as it's not strictly speaking 'internals', I thought I'd mentionned it (in particular, whatever we decide for CASSANDRA-1502 , those will have to be done). I agree that for CASSANDRA-1072 , we need some way to distinguish between cfs with counters and other cfs. But if we're just talking counters, and as far as the thrift api is concerned, it will be enough to have a 'Counter' cf type (and a 'SuperCounter' one). Or, alternatively, we could user the validateWith option to distinguish counter cfs. So I see no reason why we shouldn't remove clock_type and reconciler in this patch. Modulo the (minor) comments above, I'm +1 on this. Removing the clock interface from the internals is another matter, but I'll comment on the CASSANDRA-1502 ticket.
        Hide
        Chris Goffinet added a comment -

        If you propose removing IClock + Clock from Thrift, can you provide an alternative for what you have in mind on IClock work's replacement?

        Show
        Chris Goffinet added a comment - If you propose removing IClock + Clock from Thrift, can you provide an alternative for what you have in mind on IClock work's replacement?
        Hide
        Jonathan Ellis added a comment -

        the Clock interface adds a ton of wrapping/unwrapping to the "normal" mutation path (which is probably why 0.7 insert performance is virtually unchanged from 0.6 despite dropping String keys). I don't think that's the right place for this; treating increments as a special value rather than a special clock makes more sense.

        Show
        Jonathan Ellis added a comment - the Clock interface adds a ton of wrapping/unwrapping to the "normal" mutation path (which is probably why 0.7 insert performance is virtually unchanged from 0.6 despite dropping String keys). I don't think that's the right place for this; treating increments as a special value rather than a special clock makes more sense.
        Hide
        Johan Oskarsson added a comment -

        We still need a way to differentiate between timestamp and increment counter in CASSANDRA-1072. The easiest way would be to just leave the clock internals in place. In this patch we'd still need clock_type and reconciler.
        A minor detail is that the timestamp variable is still called clock in the patch.

        Show
        Johan Oskarsson added a comment - We still need a way to differentiate between timestamp and increment counter in CASSANDRA-1072 . The easiest way would be to just leave the clock internals in place. In this patch we'd still need clock_type and reconciler. A minor detail is that the timestamp variable is still called clock in the patch.
        Hide
        Jonathan Ellis added a comment -

        CASSANDRA-1502 created for removing IClock from internals

        Show
        Jonathan Ellis added a comment - CASSANDRA-1502 created for removing IClock from internals
        Hide
        Jonathan Ellis added a comment -

        (most of the bulk of this patch is removing generated Thrift code and updating the system tests to remove the Clock wrappers)

        Show
        Jonathan Ellis added a comment - (most of the bulk of this patch is removing generated Thrift code and updating the system tests to remove the Clock wrappers)

          People

          • Assignee:
            Jonathan Ellis
            Reporter:
            Jonathan Ellis
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development