Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-2407

Compaction thread should try to empty a bucket before moving on

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Fix Version/s: 1.1.0
    • Component/s: None
    • Labels:

      Description

      As suggested by Aaron Morton (1), a compaction thread should attempt to empty a bucket before moving on to a larger bucket. This would change the submitMinorIfNeeded for loop into a while loop that regenerated the buckets and started from the bottom after each successful compaction.

      1. 2407-v4.txt
        20 kB
        Jonathan Ellis
      2. 2407-v3.txt
        18 kB
        Jonathan Ellis
      3. 2407-v2.txt
        18 kB
        Jonathan Ellis
      4. 2407.txt
        1 kB
        Jonathan Ellis

        Activity

        Hide
        jbellis Jonathan Ellis added a comment -

        No. The benefit is too small to be worth the risk of introducing a regression.

        Show
        jbellis Jonathan Ellis added a comment - No. The benefit is too small to be worth the risk of introducing a regression.
        Hide
        hsn Radim Kolar added a comment -

        Can be this merged to 1.0 ?

        Show
        hsn Radim Kolar added a comment - Can be this merged to 1.0 ?
        Hide
        hudson Hudson added a comment -

        Integrated in Cassandra #1213 (See https://builds.apache.org/job/Cassandra/1213/)
        update size-tiered compaction to prioritize small tiers
        patch by jbellis; reviewed by slebresne for CASSANDRA-2407

        jbellis : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1203729
        Files :

        • /cassandra/trunk/CHANGES.txt
        • /cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java
        • /cassandra/trunk/src/java/org/apache/cassandra/db/DataTracker.java
        • /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java
        • /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/CompactionManager.java
        • /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java
        • /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java
        • /cassandra/trunk/test/unit/org/apache/cassandra/db/compaction/CompactionsTest.java
        Show
        hudson Hudson added a comment - Integrated in Cassandra #1213 (See https://builds.apache.org/job/Cassandra/1213/ ) update size-tiered compaction to prioritize small tiers patch by jbellis; reviewed by slebresne for CASSANDRA-2407 jbellis : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1203729 Files : /cassandra/trunk/CHANGES.txt /cassandra/trunk/src/java/org/apache/cassandra/db/ColumnFamilyStore.java /cassandra/trunk/src/java/org/apache/cassandra/db/DataTracker.java /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/AbstractCompactionStrategy.java /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/CompactionManager.java /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/LeveledCompactionStrategy.java /cassandra/trunk/src/java/org/apache/cassandra/db/compaction/SizeTieredCompactionStrategy.java /cassandra/trunk/test/unit/org/apache/cassandra/db/compaction/CompactionsTest.java
        Hide
        jbellis Jonathan Ellis added a comment -

        committed w/ change from sort to min

        Show
        jbellis Jonathan Ellis added a comment - committed w/ change from sort to min
        Hide
        jbellis Jonathan Ellis added a comment -

        Feels far-fectched that difference() would modify one of it's argument

        difference requires Set parameters, and sstables is a List.

        (arguably it should be a Set anyway, but that just moves the copyOf around, so for now it's not bugging me enough to want to make a more complex refactor).

        we could use Collections.min() rather than Collections.sort().

        good idea, I'll make that change.

        Show
        jbellis Jonathan Ellis added a comment - Feels far-fectched that difference() would modify one of it's argument difference requires Set parameters, and sstables is a List. (arguably it should be a Set anyway, but that just moves the copyOf around, so for now it's not bugging me enough to want to make a more complex refactor). we could use Collections.min() rather than Collections.sort(). good idea, I'll make that change.
        Hide
        slebresne Sylvain Lebresne added a comment -

        Two very small nitpicks while I'm at it:

        • Do we really need this ImmutableSet.copy(sstables) in DT.nonCompactingSSTables() ? Feels far-fectched that difference() would modify one of it's argument.
        • In STCS.getNextBrackgroundTask, we could use Collections.min() rather than Collections.sort().

        But otherwise lgtm, +1 (with or whithout the nits above)

        Show
        slebresne Sylvain Lebresne added a comment - Two very small nitpicks while I'm at it: Do we really need this ImmutableSet.copy(sstables) in DT.nonCompactingSSTables() ? Feels far-fectched that difference() would modify one of it's argument. In STCS.getNextBrackgroundTask, we could use Collections.min() rather than Collections.sort(). But otherwise lgtm, +1 (with or whithout the nits above)
        Hide
        jbellis Jonathan Ellis added a comment -

        v4 attached addressing above. agree on 1.1.

        Show
        jbellis Jonathan Ellis added a comment - v4 attached addressing above. agree on 1.1.
        Hide
        slebresne Sylvain Lebresne added a comment -

        Remarks:

        • getNextBackgroundTask() computes the task based on all the sstables of the CF, not only the one that are not being compacted. I believe it means that it will always return the same sstables to compact as long as those are not finished to be compacted (the submitBackground will then just say 'I can't mark as compacting so return' and we will essentially only have one compaction at a time). A simple fix would probably be to add a getNonCompactingSSTables() method to dataTracker and use that in getNextBackgroundTask.
        • submitBackground doesn't resubmit a new compaction at the end, I think we still want to do that.

        Given this ticket does have the possibility to break stuffs, I'd be more confident delaying it to 1.1.

        Show
        slebresne Sylvain Lebresne added a comment - Remarks: getNextBackgroundTask() computes the task based on all the sstables of the CF, not only the one that are not being compacted. I believe it means that it will always return the same sstables to compact as long as those are not finished to be compacted (the submitBackground will then just say 'I can't mark as compacting so return' and we will essentially only have one compaction at a time). A simple fix would probably be to add a getNonCompactingSSTables() method to dataTracker and use that in getNextBackgroundTask. submitBackground doesn't resubmit a new compaction at the end, I think we still want to do that. Given this ticket does have the possibility to break stuffs, I'd be more confident delaying it to 1.1.
        Hide
        jbellis Jonathan Ellis added a comment -

        rebased

        Show
        jbellis Jonathan Ellis added a comment - rebased
        Hide
        jbellis Jonathan Ellis added a comment -

        v2 attached (against trunk).

        I'm ok w/ putting this in 1.0.

        Show
        jbellis Jonathan Ellis added a comment - v2 attached (against trunk). I'm ok w/ putting this in 1.0.
        Hide
        dhendry Dan Hendry added a comment -

        Based on CASSANDRA-3484, is there any chance of getting this into 1.0.3 instead of 1.1?

        Show
        dhendry Dan Hendry added a comment - Based on CASSANDRA-3484 , is there any chance of getting this into 1.0.3 instead of 1.1?
        Hide
        jbellis Jonathan Ellis added a comment -

        You're right, we should really refactor the API to be getNextBackgroundTask.

        Show
        jbellis Jonathan Ellis added a comment - You're right, we should really refactor the API to be getNextBackgroundTask.
        Hide
        slebresne Sylvain Lebresne added a comment -

        I can't find where getBackgroundTasks() order the tasks by the size of the bucket (the bucket are create in a HashMap so the order is random if I read this correctly).

        Aside from that, I think that whole part of the code, with getBackgroundTasks returning a list of tasks so that we end up only compacting one of them, is a little bit ugly, but refactoring it would be substantial so I guess that's ok for now.

        Show
        slebresne Sylvain Lebresne added a comment - I can't find where getBackgroundTasks() order the tasks by the size of the bucket (the bucket are create in a HashMap so the order is random if I read this correctly). Aside from that, I think that whole part of the code, with getBackgroundTasks returning a list of tasks so that we end up only compacting one of them, is a little bit ugly, but refactoring it would be substantial so I guess that's ok for now.
        Hide
        jbellis Jonathan Ellis added a comment -

        I left it as a for loop (so we can handle the another-thread-already-compacting-this-CF case), and relies on the re-submit to get the desired behavior of compaction small sstables first. The break after one compaction avoids starving other CFs.

        Show
        jbellis Jonathan Ellis added a comment - I left it as a for loop (so we can handle the another-thread-already-compacting-this-CF case), and relies on the re-submit to get the desired behavior of compaction small sstables first. The break after one compaction avoids starving other CFs.

          People

          • Assignee:
            jbellis Jonathan Ellis
            Reporter:
            stuhood Stu Hood
            Reviewer:
            Sylvain Lebresne
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development