Cassandra
  1. Cassandra
  2. CASSANDRA-4100

Make scrub and cleanup operations throttled

    Details

      Description

      Looks like scrub and cleanup operations are not throttled and it will be nice to throttle else we are likely to run into IO issues while running it on live cluster.

      1. 0001-CASSANDRA-4100.patch
        10 kB
        Vijay
      2. 0001-CASSANDRA-4100-v2.patch
        10 kB
        Vijay
      3. 0001-CASSANDRA-4100-v3.patch
        8 kB
        Vijay

        Activity

        Hide
        Vijay added a comment -

        Attached patch adds the throttle for cleanup and scrub.
        Note: compaction_throughput_mb_per_sec is used for the throttling, not sure if making a seperate property for cleanup/scrub is better....

        Show
        Vijay added a comment - Attached patch adds the throttle for cleanup and scrub. Note: compaction_throughput_mb_per_sec is used for the throttling, not sure if making a seperate property for cleanup/scrub is better....
        Hide
        Yuki Morishita added a comment -

        Looks like patch is against trunk, though fix version is marked as 1.0.10.
        Anyway, I think Throttle object in CompcationController should be non-static since compactions may run in parallel.

        Show
        Yuki Morishita added a comment - Looks like patch is against trunk, though fix version is marked as 1.0.10. Anyway, I think Throttle object in CompcationController should be non-static since compactions may run in parallel.
        Hide
        Vijay added a comment - - edited

        >>> I think Throttle object in CompcationController should be non-static since compactions may run in parallel.
        Exactly thats why static is better, Parallel compaction is not a problem per say (ParallelCompactionIterable.getReduced() will take care of it), but compaction running one after the other (lot of small compactions).

        Let me know if everything else is ok i will rebase to 1.0.10 and move away from static (I am ok either ways), if needed. Thanks!

        Show
        Vijay added a comment - - edited >>> I think Throttle object in CompcationController should be non-static since compactions may run in parallel. Exactly thats why static is better, Parallel compaction is not a problem per say (ParallelCompactionIterable.getReduced() will take care of it), but compaction running one after the other (lot of small compactions). Let me know if everything else is ok i will rebase to 1.0.10 and move away from static (I am ok either ways), if needed. Thanks!
        Hide
        Yuki Morishita added a comment -

        OK, so static Throttle is fine here since one compaction_throughput_mb_per_sec is used for all compactions. Then, do we need to divide that by number of active compactions? I'm referring the code inside the implementation of ThroughputFunction:

        totalBytesPerMS / Math.max(1, CompactionManager.instance.getActiveCompactions());
        
        Show
        Yuki Morishita added a comment - OK, so static Throttle is fine here since one compaction_throughput_mb_per_sec is used for all compactions. Then, do we need to divide that by number of active compactions? I'm referring the code inside the implementation of ThroughputFunction: totalBytesPerMS / Math .max(1, CompactionManager.instance.getActiveCompactions());
        Hide
        Vijay added a comment -

        Fixed. Thanks!

        Show
        Vijay added a comment - Fixed. Thanks!
        Hide
        Sylvain Lebresne added a comment -

        OK, so static Throttle is fine

        I'll have to disagree. I'm pretty sure this patch break throttling. If more than one compaction share the Throttle object, they also share the Throttle.timeAtLastDelay field. Which means that as soon as there is more than 1 compaction running at any given time, the interval on which throttling is computing is bogus, and thus throttling will be bogus.

        More generally, I'm -1 on changing code that does not have any known problem on the 1.0 branch (and as far as I know, current throttling works well) as 1.0 is getting really stable and we should start being conservative there (but I'd be fine with a patch that just add throttling to scrub and cleanup for 1.0).

        Show
        Sylvain Lebresne added a comment - OK, so static Throttle is fine I'll have to disagree. I'm pretty sure this patch break throttling. If more than one compaction share the Throttle object, they also share the Throttle.timeAtLastDelay field. Which means that as soon as there is more than 1 compaction running at any given time, the interval on which throttling is computing is bogus, and thus throttling will be bogus. More generally, I'm -1 on changing code that does not have any known problem on the 1.0 branch (and as far as I know, current throttling works well) as 1.0 is getting really stable and we should start being conservative there (but I'd be fine with a patch that just add throttling to scrub and cleanup for 1.0).
        Hide
        Sylvain Lebresne added a comment -

        And to be clear, I'm not saying that throttling cannot be improved (it is true that currently it is too conservative and could throttle a thread even though the total throughput is below the threshold), but I'm saying that 1) this patch does not fix that correctly and 2) in any case this should be another ticket that shouldn't be targeted at 1.0.

        Show
        Sylvain Lebresne added a comment - And to be clear, I'm not saying that throttling cannot be improved (it is true that currently it is too conservative and could throttle a thread even though the total throughput is below the threshold), but I'm saying that 1) this patch does not fix that correctly and 2) in any case this should be another ticket that shouldn't be targeted at 1.0.
        Hide
        Vijay added a comment -

        Alright, v3 simply adds throttle to scrub and cleanup... Simple refactor to move the throttle to compaction controller.

        Show
        Vijay added a comment - Alright, v3 simply adds throttle to scrub and cleanup... Simple refactor to move the throttle to compaction controller.
        Hide
        Yuki Morishita added a comment -

        v3 looks good to me, but as Sylvain said, I'm +1 to put this to version 1.1.1 instead of 1.0.10.

        Show
        Yuki Morishita added a comment - v3 looks good to me, but as Sylvain said, I'm +1 to put this to version 1.1.1 instead of 1.0.10.
        Hide
        Vijay added a comment -

        Committed to 1.1 and trunk. Thanks!

        Show
        Vijay added a comment - Committed to 1.1 and trunk. Thanks!

          People

          • Assignee:
            Vijay
            Reporter:
            Vijay
            Reviewer:
            Yuki Morishita
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development