Details

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

      Description

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

        Activity

        Jonathan Ellis created issue -
        Jonathan Ellis made changes -
        Field Original Value New Value
        Status Open [ 1 ] Patch Available [ 10002 ]
        Chris Goffinet made changes -
        Assignee Jonathan Ellis [ jbellis ] Chris Goffinet [ lenn0x ]
        Hide
        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
        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)
        Jonathan Ellis made changes -
        Assignee Chris Goffinet [ lenn0x ] Jonathan Ellis [ jbellis ]
        Fix Version/s 0.4 [ 12313862 ]
        Hide
        Chris Goffinet added a comment -

        +1 on all 5 patches.

        Show
        Chris Goffinet added a comment - +1 on all 5 patches.
        Hide
        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
        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.
        Jonathan Ellis made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        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 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
        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
        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
        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
        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 .
        Michael Greene made changes -
        Component/s Core [ 12312978 ]
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12466099 ] patch-available, re-open possible [ 12750190 ]
        Gavin made changes -
        Workflow patch-available, re-open possible [ 12750190 ] reopen-resolved, no closed status, patch-avail, testing [ 12756172 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development