Allow transient range owner to serve as repair coordinator by ifesdjeen · Pull Request #257 · apache/cassandra

ifesdjeen · 2018-09-05T17:49:39Z

…f classes

aweisberg · 2018-09-05T21:41:23Z

src/java/org/apache/cassandra/repair/RepairRunnable.java

        {
            // Use strict equality here, as worst thing that can happen is we generate one more stream
-            if (Iterables.elementsEqual(cr.endpoints, endpoints) && Iterables.elementsEqual(cr.transEndpoints, transEndpoints))
+            if (Sets.difference(cr.endpoints, endpoints).isEmpty() && Sets.difference(cr.transEndpoints, transEndpoints).isEmpty())


You can use .equals() here. Set equality does the right thing.

Right, return s.size() == o.size() && s.containsAll(o);, i should've did that from starters.

bdeggleston · 2018-09-05T23:28:21Z

I think I've mostly convinced myself this is all you need to do to support coordinating repairs for locally transient ranges. I have a commit here here with a few adjustments:

Move set comparison logic in RepairRunnable#addRangeToNeighbors to CommonRange class
changed asserts to Preconditions
added a SymmetricLocalSync task test for the local transient case
misc formatting

This leaves the sync task class hierarchy in kind of a messy state. We have AsymmetricSyncTask, but we also have SymmetricSyncTask which can also function as an asymmetric sync task. I do think the state of things is preferable to adding more branches to RepairJob#standardSyncing though, and a refactor is out of scope for this ticket. I'll open a follow on jira to clean that up once this is committed.

ifesdjeen · 2018-09-06T13:53:49Z

@bdeggleston thank you for the review and changes, they look great!

I agree the class hierarchy became not very optimal. I was concentrating on the AssumetrciLocalSyncTask and didn't change the symmetric one. I've added a follow-up commit that makes only one of them necessary and renames it to LocalSyncTask. It did not seem necessary to do the same with remote tasks as there we do not depend on the argument order, which we do in the local case because of transiency. I did try to avoid this change but couldn't find a good way to make it order-independent, but I've changed argument names and made sure corresponding checks are in place.

aweisberg · 2018-09-06T18:05:30Z

src/java/org/apache/cassandra/repair/LocalSyncTask.java

            // send ranges to the remote node if we are not performing a pull repair
            // see comment on RangesAtEndpoint.toDummyList for why we synthesize replicas here
-            plan.transferRanges(dst, desc.keyspace, RangesAtEndpoint.toDummyList(differences), desc.columnFamily);
+            plan.transferRanges(remote, desc.keyspace, RangesAtEndpoint.toDummyList(differences), desc.columnFamily);


So the idiom where it returns this isn't always used to return "this" some of the time it's a copy. Granted here it is returning this, but should you assign the plan field on the stack again?

I don't feel strongly about it just noticing that.

I felt like this is a side-effect generating, imperative method, so ignoring its return is fine as it's done in order to facilitate chaining.

aweisberg · 2018-09-06T18:07:14Z

src/java/org/apache/cassandra/repair/RepairJob.java

+                    // pull only if local is full
+                    boolean requestRanges = !isTransient(self.endpoint);
+                    // push only if remote is full; additionally check for pull repair
+                    boolean transfterRanges = !isTransient(remote.endpoint) && !session.pullRepair;


Typo in transfer

Thanks for noticing!

aweisberg · 2018-09-06T18:19:43Z

src/java/org/apache/cassandra/service/ActiveRepairService.java

 import java.net.UnknownHostException;
-import java.util.*;
-import java.util.concurrent.*;
+import java.util.ArrayList;


Import fighting still happening?

Yes, I'll remove it on commit when it's squashed.

ifesdjeen · 2018-09-06T21:13:47Z

@aweisberg fixed imports and spelling. Thank you for spotting those.

bdeggleston · 2018-09-07T18:00:12Z

@ifesdjeen did some further consolidation of the sync task class hierarchy here. I'm not really opposed to doing this refactor here instead of a separate ticket, but @krummas should probably take a look now :)

ifesdjeen · 2018-09-07T18:55:42Z

Heh, now I need to take a look as well)

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method.

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method. (cherry picked from commit 47d0719)

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method. (cherry picked from commit 47d0719) (cherry picked from commit 6ed9a94)

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method. (cherry picked from commit 47d0719) (cherry picked from commit 6ed9a94) (cherry picked from commit eaed731)

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method. (cherry picked from commit 47d0719) (cherry picked from commit 6ed9a94) (cherry picked from commit eaed731) (cherry picked from commit 4432e3c) (+1 squashed commit) Squashed commits: [5d65744ea5b] STAR-845: Port MessagingService implementation from DSE (apache#254) Add support for: - Registering new verbs at runtime - Decorating existing verb handlers with another method - Running a callback after the MessagingService sends a message (cherry picked from commit c7d0ba0) (cherry picked from commit e8e13ed) (cherry picked from commit 964892a) (cherry picked from commit 1968d3c) (cherry picked from commit 012cd01)

FailingRepairTest uses serialization to pass Verbs back and forth between the nodes during the test. Unforuntately, Verbs aren't serializable anymore because they're no longer enums and this broke the test. Instead of passing a verb around, pass the verb Id and lookup the verb inside of the test method. (cherry picked from commit 47d0719) (cherry picked from commit 6ed9a94) (cherry picked from commit eaed731) (cherry picked from commit 4432e3c) (+1 squashed commit) Squashed commits: [5d65744ea5b] STAR-845: Port MessagingService implementation from DSE (apache#254) Add support for: - Registering new verbs at runtime - Decorating existing verb handlers with another method - Running a callback after the MessagingService sends a message (cherry picked from commit c7d0ba0) (cherry picked from commit e8e13ed) (cherry picked from commit 964892a) (cherry picked from commit 1968d3c) (cherry picked from commit 012cd01) (cherry picked from commit 67b9707) STAR-881 Fix rebase compile issues

ifesdjeen and others added 7 commits September 5, 2018 15:12

Imrpve DiskBoundaryManager, bring it into consistency with the rest o…

aadc3c6

…f classes

Improve repair scheduling

c1a73a5

Avoid filtering already

e47019f

Allow transient range owner to be repair coordinator

3e61a44

enable dtests

1d19e26

Switch to branch that has TR tests

c9d4e02

Use Sets#difference instead of per-element equality

07acea8

aweisberg reviewed Sep 5, 2018

View reviewed changes

Alex Petrov and others added 4 commits September 6, 2018 10:27

Attempt to reduce class hierarchy

31f709c

Fix SymmetricRemoteSyncTaskTest

7cc36f2

misc updates

592cf72

Rename test, too

389223a

Cosmetics

f4e1f3f

aweisberg reviewed Sep 6, 2018

View reviewed changes

Fix Ariel’s comments

090b224

ifesdjeen closed this Sep 12, 2018

Conversation

ifesdjeen commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg Sep 5, 2018

Choose a reason for hiding this comment

Uh oh!

ifesdjeen Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdeggleston commented Sep 5, 2018

Uh oh!

ifesdjeen commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aweisberg Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

ifesdjeen Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

aweisberg Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

ifesdjeen Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

aweisberg Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

ifesdjeen Sep 6, 2018

Choose a reason for hiding this comment

Uh oh!

ifesdjeen commented Sep 6, 2018

Uh oh!

bdeggleston commented Sep 7, 2018

Uh oh!

ifesdjeen commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ifesdjeen commented Sep 5, 2018 •

edited

Loading

ifesdjeen Sep 5, 2018 •

edited

Loading

ifesdjeen commented Sep 6, 2018 •

edited

Loading