Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Invalid
    • Fix Version/s: None
    • Component/s: Core
    • Labels:
      None

      Description

      In order to make CASSANDRA-1072 easier to digest we should split out context related functionality and prepare the code base for multiple clock types.

        Issue Links

          Activity

          Hide
          Johan Oskarsson added a comment -

          Changes include the following

          • add context byte array to avro/thrift interface
          • validate data using a clock specific validator method (to avoid negative deltas in incr counters for example)
          • pull diff, update and cleanContext code into the clocks
          • prepare CliClient for multiple clock types
          • enable aes code to clean contexts in a clock specific manner
          • prepare anti entropy test to run against counter columns
          Show
          Johan Oskarsson added a comment - Changes include the following add context byte array to avro/thrift interface validate data using a clock specific validator method (to avoid negative deltas in incr counters for example) pull diff, update and cleanContext code into the clocks prepare CliClient for multiple clock types enable aes code to clean contexts in a clock specific manner prepare anti entropy test to run against counter columns
          Hide
          Johan Oskarsson added a comment -

          Updated to latest trunk.

          Show
          Johan Oskarsson added a comment - Updated to latest trunk.
          Hide
          Sylvain Lebresne added a comment -

          First off, I do like the idea of splitting the CASSANDRA-1072 patch to make it
          easier to work with. However I think that the introduction of at least
          Clock.cleanContext and Clock.update() is really hard to judge based on this
          patch alone (but I have some reserve below on those two anyway).

          Also note that parts of the rest of this comments is probably more about
          CASSANDRA-1072 than CASSANDRA-1243, but I'm too lazy to sort my comments

          I don't know where Kelvin is with the replication parts of the increment
          counters. As far as I understand things, solving the replication may amounts
          to be able to distinguish between writes and repairs. If so, I believe
          cleanContext will not be useful anymore, since nodes will be able to ignore
          the count concerning themselves for repairs (unless I miss something). This
          would be good btw since that would remove the changes to the AntiCompaction
          stuffs and prevent us to mess up by forgetting some cleanContext.

          In SP.mutate() and SP.mutateBlocking(), rm.updateClocks(destination) is
          called. This increments by 1 the clock count for the destination. Not sure why
          it's done, seems wrong to me. If I'm right, I believe the IClock.update() can
          go away, at least for as far as CASSANDRA-1072 is concerned.

          Also, shouldn't we get rid of the binary context in thrift/avro (until we have
          actual version clock) ? About the API, not sure what to do but we should do
          something. A suggestion could be to add a thrift enum 'ClockType' (that, btw,
          could be reused in the CfDef structure instead of raw string). A clock would
          then be a required ClockType followed by optional other fields that would
          depend on the clock type (increment counters wouldn't need any such optional
          fields).

          Last minor comments, there is a missing validateValueByClock in the SC case of
          ThriftValidation.validateColumnOrSuperColumn().

          Show
          Sylvain Lebresne added a comment - First off, I do like the idea of splitting the CASSANDRA-1072 patch to make it easier to work with. However I think that the introduction of at least Clock.cleanContext and Clock.update() is really hard to judge based on this patch alone (but I have some reserve below on those two anyway). Also note that parts of the rest of this comments is probably more about CASSANDRA-1072 than CASSANDRA-1243 , but I'm too lazy to sort my comments I don't know where Kelvin is with the replication parts of the increment counters. As far as I understand things, solving the replication may amounts to be able to distinguish between writes and repairs. If so, I believe cleanContext will not be useful anymore, since nodes will be able to ignore the count concerning themselves for repairs (unless I miss something). This would be good btw since that would remove the changes to the AntiCompaction stuffs and prevent us to mess up by forgetting some cleanContext. In SP.mutate() and SP.mutateBlocking(), rm.updateClocks(destination) is called. This increments by 1 the clock count for the destination. Not sure why it's done, seems wrong to me. If I'm right, I believe the IClock.update() can go away, at least for as far as CASSANDRA-1072 is concerned. Also, shouldn't we get rid of the binary context in thrift/avro (until we have actual version clock) ? About the API, not sure what to do but we should do something. A suggestion could be to add a thrift enum 'ClockType' (that, btw, could be reused in the CfDef structure instead of raw string). A clock would then be a required ClockType followed by optional other fields that would depend on the clock type (increment counters wouldn't need any such optional fields). Last minor comments, there is a missing validateValueByClock in the SC case of ThriftValidation.validateColumnOrSuperColumn().
          Hide
          Kelvin Kakugawa added a comment -

          In 1072, I've already worked out the logic to extend counter contexts w/ bit flags and perform secondary counter writes in SP.mutateBlocking(). Still needs to be tested, though.

          After the above changeset is vetted, we should go through and start removing all the "clean"-related logic. None of us were fans of that logic, either, and it'll definitely reduce the size of this patch against trunk.

          Show
          Kelvin Kakugawa added a comment - In 1072, I've already worked out the logic to extend counter contexts w/ bit flags and perform secondary counter writes in SP.mutateBlocking(). Still needs to be tested, though. After the above changeset is vetted, we should go through and start removing all the "clean"-related logic. None of us were fans of that logic, either, and it'll definitely reduce the size of this patch against trunk.

            People

            • Assignee:
              Unassigned
              Reporter:
              Johan Oskarsson
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development