Details

      Description

      As was noticed in CASSANDRA-7317, using '-pr' alongside '-local' for repair doesn't really work properly, and disabling the combination was definitively the right short time fix. However, the main goal of '-pr' is to make it easy to repair a full cluster without doing any duplication of work. Doing the same only within a data-center is obviously desirable.

      I think a reasonably simple solution would be modify the behavior of '-pr' when it's limited to only one DC. If applied to nodeX in dcY, instead of repairing only the "primary" range of nodeX for the whole ring, we'll repair that range but also all ranges that are "primary" for a node not in dcY and for which nodeX is the first node of dcY found in ring order. Basically we'll ensure that running 'repair -local -pr' on every nodes of a given DC will repair all ranges for the nodes of that DC without repairing the same range twice.

      1. 2.1-CASSANDRA-7450-v1.txt
        10 kB
        Paulo Motta
      2. 2.1-CASSANDRA-7450-v2.txt
        20 kB
        Paulo Motta
      3. 7450-2.1-v3.txt
        27 kB
        Yuki Morishita

        Issue Links

          Activity

          Hide
          pauloricardomg Paulo Motta added a comment -

          Initial version to validate idea. No tests yet.

          Also available on: https://github.com/chaordic/cassandra/commit/75b0a962585ab0fb8208608e2692edb09694bc25

          @cc Yuki Morishita

          Show
          pauloricardomg Paulo Motta added a comment - Initial version to validate idea. No tests yet. Also available on: https://github.com/chaordic/cassandra/commit/75b0a962585ab0fb8208608e2692edb09694bc25 @cc Yuki Morishita
          Hide
          yukim Yuki Morishita added a comment -
          • I think it is better to name getPrimaryRangeForEndpointWithinDC than getLocalPrimaryRangeForEndpoint
          • I think you can simplify this to include Range s that given referenceEndpoint is not the primary endpoint but is replica for that range. That is:
          List<InetAddress> endpoints = strategy.calculateNaturalEndpoints(token, metadata);
          if (endpoints.size() > 0 && !endpoints.get(0).equals(ep) && endpoints.contains(ep))
              primaryRanges.add(new Range<>(metadata.getPredecessor(token), token));
          
          • Wrapped range is handled in Range object so you don't need last for loop. (Sorry if I told it wrong.)
          • Nice if you can add unit test for getLocalPrimaryRangeForEndpoint to StorageServiceServerTest like getPrimaryRangeForEndpoint.
          Show
          yukim Yuki Morishita added a comment - I think it is better to name getPrimaryRangeForEndpointWithinDC than getLocalPrimaryRangeForEndpoint I think you can simplify this to include Range s that given referenceEndpoint is not the primary endpoint but is replica for that range. That is: List<InetAddress> endpoints = strategy.calculateNaturalEndpoints(token, metadata); if (endpoints.size() > 0 && !endpoints.get(0).equals(ep) && endpoints.contains(ep)) primaryRanges.add( new Range<>(metadata.getPredecessor(token), token)); Wrapped range is handled in Range object so you don't need last for loop. (Sorry if I told it wrong.) Nice if you can add unit test for getLocalPrimaryRangeForEndpoint to StorageServiceServerTest like getPrimaryRangeForEndpoint .
          Hide
          pauloricardomg Paulo Motta added a comment -

          V2 version of the patch attached with the proposed changes and tests.

          Is backport to 2.0 needed or it's mergeable with 2.0? I will provide the backport to 1.2, since this might be an important feature to many 1.2 users without risking existing functionality.

          Show
          pauloricardomg Paulo Motta added a comment - V2 version of the patch attached with the proposed changes and tests. Is backport to 2.0 needed or it's mergeable with 2.0? I will provide the backport to 1.2, since this might be an important feature to many 1.2 users without risking existing functionality.
          Hide
          brandon.williams Brandon Williams added a comment -

          Don't bother with 1.2, it's done.

          Show
          brandon.williams Brandon Williams added a comment - Don't bother with 1.2, it's done.
          Hide
          yukim Yuki Morishita added a comment -

          Thanks, Paulo Motta!
          I attached V3 version which is based on your work.
          There are some places that are checking presence of bothe "-pr" and "-local", so I modified check to actually run repair in that case.
          Plus, some code clean up.

          Is this looking good?

          Show
          yukim Yuki Morishita added a comment - Thanks, Paulo Motta ! I attached V3 version which is based on your work. There are some places that are checking presence of bothe "-pr" and "-local", so I modified check to actually run repair in that case. Plus, some code clean up. Is this looking good?
          Hide
          pauloricardomg Paulo Motta added a comment -

          LGTM, thanks!

          Show
          pauloricardomg Paulo Motta added a comment - LGTM, thanks!
          Hide
          yukim Yuki Morishita added a comment -

          Committed, thanks!

          Show
          yukim Yuki Morishita added a comment - Committed, thanks!

            People

            • Assignee:
              pauloricardomg Paulo Motta
              Reporter:
              slebresne Sylvain Lebresne
              Reviewer:
              Yuki Morishita
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development