Details

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

      Description

      patches 01, 02, and 03 that I created for CASSANDRA-79 apply here and can be reviewed separately.

        Activity

        Hide
        jbellis Jonathan Ellis added a comment -

        New patch set to add test and fix bug.

        02 is the new test.

        05 is the bugfix (will be squashed into 04, the refactor, for actual commit but if you'd looked at the old 03, splitting the fix out separately makes it more clear what changed)

        Show
        jbellis Jonathan Ellis added a comment - New patch set to add test and fix bug. 02 is the new test. 05 is the bugfix (will be squashed into 04, the refactor, for actual commit but if you'd looked at the old 03, splitting the fix out separately makes it more clear what changed)
        Hide
        lenn0x Chris Goffinet added a comment -

        +1 on all 5 patches.

        Show
        lenn0x Chris Goffinet added a comment - +1 on all 5 patches.
        Hide
        jbellis Jonathan Ellis added a comment -

        i'm going to commit these based on Chris's +1 so we can make progress on the rest of CASSANDRA-79, but I'm still interested in Jun's feedback if any.

        Show
        jbellis Jonathan Ellis added a comment - i'm going to commit these based on Chris's +1 so we can make progress on the rest of CASSANDRA-79 , but I'm still interested in Jun's feedback if any.
        Hide
        hudson Hudson added a comment -

        Integrated in Cassandra #113 (See http://hudson.zones.apache.org/hudson/job/Cassandra/113/)
        switch from byte[] to BitSet in CommitLogHeader. patch by jbellis; reviewed by goffinet for
        update comments, perform some renames, r/m unused code.
        patch by jbellis; reviewed by goffinet for
        add CommitLog and RecoveryManager tests. patch by jbellis; reviewed by goffinet for

        Show
        hudson Hudson added a comment - Integrated in Cassandra #113 (See http://hudson.zones.apache.org/hudson/job/Cassandra/113/ ) switch from byte[] to BitSet in CommitLogHeader. patch by jbellis; reviewed by goffinet for update comments, perform some renames, r/m unused code. patch by jbellis; reviewed by goffinet for add CommitLog and RecoveryManager tests. patch by jbellis; reviewed by goffinet for
        Hide
        junrao Jun Rao added a comment -

        Looked at the patch. It looks fine overall. A few comments:

        1. In CL.maybeUpdateHeader(), why are there 2 identical if tests?

        2. In CL.discard(), the following should never happen. Otherwise, the recovery logic won't be right. So, we probably should change the if test to as assertion.
        if ( cLogCtx.position < commitLogHeader.getPosition(id) )

        3. This is an optimization and should be left to another jira if we want to pursue this. I don't quite understand why a new CL has to inherit the dirty bits from the old one. If during the lifetime of a CL, a CF is not updated, its bit can be left off. This allows CLs to be GCed early.

        Show
        junrao Jun Rao added a comment - Looked at the patch. It looks fine overall. A few comments: 1. In CL.maybeUpdateHeader(), why are there 2 identical if tests? 2. In CL.discard(), the following should never happen. Otherwise, the recovery logic won't be right. So, we probably should change the if test to as assertion. if ( cLogCtx.position < commitLogHeader.getPosition(id) ) 3. This is an optimization and should be left to another jira if we want to pursue this. I don't quite understand why a new CL has to inherit the dirty bits from the old one. If during the lifetime of a CL, a CF is not updated, its bit can be left off. This allows CLs to be GCed early.
        Hide
        jbellis Jonathan Ellis added a comment - - edited

        I agree on all counts. Re 3 I think this had to do with the "try to stop replay early" code dealing with Stacks of CommitLogs that I removed.

        I will attach patch to CASSANDRA-243.

        Show
        jbellis Jonathan Ellis added a comment - - edited I agree on all counts. Re 3 I think this had to do with the "try to stop replay early" code dealing with Stacks of CommitLogs that I removed. I will attach patch to CASSANDRA-243 .

          People

          • Assignee:
            jbellis Jonathan Ellis
            Reporter:
            jbellis Jonathan Ellis
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development