STAR-842: Port pluggable Failure Detector by mfleming · Pull Request #237 · datastax/cassandra

mfleming · 2021-08-19T15:56:55Z

The failure detector is now configurable via cassandra.custom_failure_detector_class

k-rus

Looks good to my best ability. Just some nits.

I have few comments, which is up to you if you think it is worth to fix:

There is duplicated code between tests to initialise a ring. It might be good to share it instead of duplicating. This can help to read tests and maintain them in future.
There is an inconsistency in comments where some comments start with capital letter and others not. I guess the code style tools do not support fixing such formatting.

k-rus · 2021-08-27T08:46:00Z

+    }
+
+    @Test
+    public void testStateBootReplacingFailsForLiveNode() throws UnknownHostException


It seems that the initialisation code, which creates a ring, is the same between the tests. It might be good to replace duplicated code with a single function call if possible.
It is a suggestion, but not required :)

This is a good idea but I couldn't come up with a scheme that I liked since we need to access various objects both inside the unified method and in the test-specific bits so we'd either end up passing around containers of stuff or wrap everything in another object.

k-rus · 2021-08-27T08:48:08Z

+    }
+
+    @Test
+    public void testReplacingLiveNodeFails() throws UnknownHostException


Same comment about initialisation code, which creates the ring.

k-rus · 2021-08-27T09:30:59Z

+
+        SchemaMigrationEvent e3 = new SchemaMigrationEvent(SchemaMigrationEvent.MigrationManagerEventType.TASK_CREATED, deadEndpoint, null);
+    }
+}


Since I am new to Java, I don't know if it is expected to have a new line at the end of a file. I guess such things would be automatically resolved by formatting tool.

I generally leave this up to my IDE which seems to insert newlines at the end of files and is a common pattern in cassandra -- probably because the majority of us are using IntelliJ.

But here it wasn't inserted and in another new file :)

jacek-lewandowski

Some comments from me as well, feel free to apply or ignore, there is nothing critical

jacek-lewandowski · 2021-08-27T10:08:25Z

+    MBEAN_REGISTRATION_CLASS("org.apache.cassandra.mbean_registration_class"),
+
+    /** Which class to use for failure detection */
+    CUSTOM_FAILURE_DETECTOR_PROPERTY("cassandra.custom_failure_detector_class");


maybe we could follow the convention and prepend the property name with org.apache.? wdyt?

We could do this but it will break CNDB's existing configs and is probably better to be done separately from this port. I'll open another ticket so we can coordinate this name change.

I suppose that CNDB will have a separate configuration for Converged Cassandra anyway. @JeremiahDJordan wdyt about this kind of changes?

For reference, this option is currently embedded in the code (not an external config file) for the CNDB writer and CNDB coordinator.

jacek-lewandowski · 2021-08-27T10:11:16Z


 public interface IFailureDetector
 {
+    IFailureDetector instance = CUSTOM_FAILURE_DETECTOR_PROPERTY.getString() == null ?


Maybe instead of this and CustomFailureDetector we could have a FailureDetectorFactory which would provide the instance of the appropriate implementation?

Sure. What would be the benefits? A simpler interface for the initialising the instance field?

Well, factory is the design pattern which is used to obtain the certain implementation

jacek-lewandowski · 2021-08-27T10:12:15Z

+                                new FailureDetector() :
+                                CustomFailureDetector.make(CUSTOM_FAILURE_DETECTOR_PROPERTY.getString());
+
+    public static final Predicate<InetAddressAndPort> isEndpointAlive = instance::isAlive;


Could those two guys not be static?

jacek-lewandowski · 2021-08-27T10:19:01Z

        }

-        if (FailureDetector.instance.isAlive(oldNode))
+        if (IFailureDetector.instance.isAlive(oldNode))


General comment for this file - if we made those methods which access failure detector non-static, we could have a final field with the failure detector instance which is initialized in the constructor of StorageService. If at some point we wanted to test StorageService class in isolation, we could provide a mock for the failure detector and made make it easier to test

I like this idea, but I'd like it even more if we made the change at the same time as we tested StorageService in isolation rather than making the tweak now in anticipation of future changes.

Co-authored-by: Ruslan Fomkin <Ruslan.Fomkin@gmail.com>

sonarqubecloud · 2021-08-27T13:17:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

70.3% Coverage
0.0% Duplication

JeremiahDJordan · 2021-08-27T14:49:59Z

Agree with minimal change for the initial porting.

The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439)

The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959)

… port (#245) (cherry picked from commit adf34e3) (cherry picked from commit f2a9b43) (cherry picked from commit 68ea688) (cherry picked from commit 4c04a81) (+1 squashed commit) Squashed commits: [47787240498] STAR-842: Port pluggable Failure Detector (#237) The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959)

… port (#245) (cherry picked from commit adf34e3) (cherry picked from commit f2a9b43) (cherry picked from commit 68ea688) (cherry picked from commit 4c04a81) (+1 squashed commit) Squashed commits: [47787240498] STAR-842: Port pluggable Failure Detector (#237) The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959) (cherry picked from commit 294407b)

… port (#245) (cherry picked from commit adf34e3) (cherry picked from commit f2a9b43) (cherry picked from commit 68ea688) (cherry picked from commit 4c04a81) (+1 squashed commit) Squashed commits: [47787240498] STAR-842: Port pluggable Failure Detector (#237) The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959) (cherry picked from commit 294407b) (cherry picked from commit 5fb2559)

… port (#245) (cherry picked from commit adf34e3) (cherry picked from commit f2a9b43) (cherry picked from commit 68ea688) (cherry picked from commit 4c04a81) (+1 squashed commit) Squashed commits: [47787240498] STAR-842: Port pluggable Failure Detector (#237) The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959) (cherry picked from commit 294407b) (cherry picked from commit 5fb2559) STAR-868 Fix rebase compile issues STAR-868 Fix test failures with InstanceAlreadyExistsException for FailureDetector due to bad cherry-pick conflict resolution in FailureDetector

… port (#245) (cherry picked from commit adf34e3) (cherry picked from commit f2a9b43) (cherry picked from commit 68ea688) (cherry picked from commit 4c04a81) (+1 squashed commit) Squashed commits: [47787240498] STAR-842: Port pluggable Failure Detector (#237) The failure detector is now configurable via cassandra.custom_failure_detector_class (cherry picked from commit 8127d43) (cherry picked from commit d9625b6) (cherry picked from commit 3197439) (cherry picked from commit dc27959) (cherry picked from commit 294407b) (cherry picked from commit 5fb2559) STAR-868 Fix rebase compile issues STAR-868 Fix test failures with InstanceAlreadyExistsException for FailureDetector due to bad cherry-pick conflict resolution in FailureDetector (Rebase of commit 0f828a0)

mfleming added 15 commits August 19, 2021 16:55

STAR-842: Port pluggable Failure Detector

65e56a2

The failure detector is now configurable via cassandra.custom_failure_detector_class

Fix license

3d07a73

Add more test to make SonarCloud happy

02e4c0e

Add RepairSessionTest for isAlive

8baf5ad

More tests

a028484

Moar tests

9ce468d

EVEN MOAR TESTS

5e0063c

Moar tests

9619db0

And again more tests

8aee1b7

Gossip tests

8d97088

moooore tests

efa6c3e

Add CustomFailureDetector constructor test

bac6bad

SchemaMigrationEvent test

b24ffd5

Refactor code to mark nodes as dead or join ring

b433412

Make CustomFailureDetector ctor private

77efb8c

mfleming requested review from JeremiahDJordan and k-rus and removed request for JeremiahDJordan August 26, 2021 13:20

k-rus approved these changes Aug 27, 2021

View reviewed changes

jacek-lewandowski reviewed Aug 27, 2021

View reviewed changes

mfleming and others added 6 commits August 27, 2021 11:33

Update test/unit/org/apache/cassandra/gms/FailureDetectorTest.java

2c21fbf

Co-authored-by: Ruslan Fomkin <Ruslan.Fomkin@gmail.com>

Update test/unit/org/apache/cassandra/gms/GossiperTest.java

396c0b3

Co-authored-by: Ruslan Fomkin <Ruslan.Fomkin@gmail.com>

start comments with capital letters

46f411e

Oops, fix build failure

95dd83d

Comment from Ruslan

873f85b

Ruslan feedback

68098c5

mfleming merged commit e401f9a into ds-trunk Aug 27, 2021

mfleming deleted the STAR-842 branch August 27, 2021 14:17

Conversation

mfleming commented Aug 19, 2021

Uh oh!

k-rus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jacek-lewandowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfleming Aug 27, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 27, 2021

Uh oh!

JeremiahDJordan commented Aug 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mfleming Aug 27, 2021 •

edited

Loading