[DSIP-9][registry] add raft registration plugin #16352

MatthewAden · 2024-07-20T12:49:03Z

Purpose of the pull request

This is a project for the OSPP 2024. This PR is close #10874

Brief change log

Added a new module: /dolphinscheduler-registry/dolphinscheduler-plugins/dolphinscheduler-registry-raft
Added raft's dependency in dolphinscheduler-bom/pom.xml
Added dolphinscheduler-registry-raft dependency in dolphinscheduler-registry/dolphinscheduler-registry-all/pom.xml

Verify this pull request

Unit Test Results:

Pseudo-Cluster Deployment Status:

Cluster Test

Test conditions:
2 master nodes, 1 worker node
OS: mac
Steps:
Start node1, node2, and node3 in sequence

Be able to normally elect a Leader
When the Leader of the cluster crashes, be able to normally elect a new Leader
The original crashed Leader re-joins the cluster and is automatically downgraded to a Follower

boring-cyborg · 2024-07-20T12:49:06Z

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

...cheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-raft/README.md

...gistry-raft/src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftRegistry.java

...src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftSubscribeDataManager.java

...cheduler-registry-plugins/dolphinscheduler-registry-raft/src/test/resources/application.yaml

...c/main/java/org/apache/dolphinscheduler/plugin/registry/raft/client/IRaftRegisterClient.java

...c/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftConnectionStateManager.java

...src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftSubscribeDataManager.java

...c/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftConnectionStateManager.java

...try-raft/src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftLockManager.java

...gistry-raft/src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftRegistry.java

ruanwenjun · 2024-08-19T06:34:26Z

...rc/main/java/org/apache/dolphinscheduler/plugin/registry/raft/client/RaftRegistryClient.java

+        if (raftRegistryProperties.getModule().equals(MASTER_MODULE)) {
+            raftSubscribeDataManager.start();
+        }


Why only master start the raftSubscribeDataManager, this is strange.

In my understanding, only the master listens for changes in the registered instances

ruanwenjun · 2024-08-19T06:36:25Z

...ain/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftRegistryAutoConfiguration.java

+    @Bean
+    @DependsOn("raftRegistryServer")
+    @ConditionalOnProperty(prefix = "registry", name = "module", havingValue = "master")
+    public RaftRegistry masterRaftRegistryClient(RaftRegistryProperties raftRegistryProperties) {
+        RaftRegistry raftRegistry = new RaftRegistry(raftRegistryProperties);
+        raftRegistry.start();
+        return raftRegistry;
+    }
+
+    @Bean
+    @ConditionalOnProperty(prefix = "registry", name = "module", havingValue = "worker")
+    public RaftRegistry workerRaftRegistryClient(RaftRegistryProperties raftRegistryProperties) {
+        RaftRegistry raftRegistry = new RaftRegistry(raftRegistryProperties);
+        raftRegistry.start();
+        return raftRegistry;
+    }
+
+    @Bean
+    @ConditionalOnProperty(prefix = "registry", name = "module", havingValue = "api")
+    public RaftRegistry apiRaftRegistryClient(RaftRegistryProperties raftRegistryProperties) {
+        RaftRegistry raftRegistry = new RaftRegistry(raftRegistryProperties);
+        raftRegistry.start();
+        return raftRegistry;
+    }


The plugin shouldn't care about the server type.

I want to achieve is that the master start the client and server of raft, while other modules only need to start the client of raft. Is there a better way to achieve this?

The plugin doesn't need to care about the server type, we will +/- server type in the future.
And if only the master start the server, this means if the cluster doesn't exist masters, then the registry will died? this is not accepted.

we can start raft registry in a separate process like other server

we can start raft registry in a separate process like other server

The main purpose of this feature is to reduce dependence on third-party components. Starting raft registry in a separate process is exactly the opposite of what we wanted. We should add to each server and be insensitive to existing users as the service starts.

we can start raft registry in a separate process like other server

The main purpose of this feature is to reduce dependence on third-party components. Starting raft registry in a separate process is exactly the opposite of what we wanted. We should add to each server and be insensitive to existing users as the service starts.

YES，We will add raft registry server to master server and worker server. All the server node participate in the election of the raft leader node.This will also make the cluster more rubust.

The modification has been completed. All nodes will form nodes in the raft cluster instead of only the master forming the raft cluster.

ruanwenjun · 2024-08-19T06:37:19Z

...t/src/main/java/org/apache/dolphinscheduler/plugin/registry/raft/RaftRegistryProperties.java

+    private String clusterName;
+    private String serverAddressList;
+    private String serverAddress;
+    private int serverPort;
+    private String logStorageDir;
+    private Duration distributedLockTimeout = Duration.ofSeconds(3);
+    private Duration distributedLockRetryInterval = Duration.ofSeconds(5);
+    private String module = "master";
+    private Duration listenerCheckInterval = Duration.ofSeconds(3);
+    private int cliMaxRetries = 3;
+    private Duration cliTimeout = Duration.ofSeconds(5);
+    private Duration refreshLeaderTimeout = Duration.ofSeconds(2);
+    private Duration connectStateCheckInterval = Duration.ofSeconds(2);
+    private Duration heartBeatTimeOut = Duration.ofSeconds(20);
+    private int subscribeListenerThreadPoolSize = 1;
+    private int connectionListenerThreadPoolSize = 1;


Why we need to exposed so many configurations to user. Is there any doc related to these config?

The following configurations are the JRaft component use.

private String clusterName; private String serverAddressList; private String serverAddress; private int serverPort; private String logStorageDir; private Duration distributedLockTimeout = Duration.ofSeconds(3); private int cliMaxRetries = 3; private Duration cliTimeout = Duration.ofSeconds(5); private Duration refreshLeaderTimeout = Duration.ofSeconds(2);

The following are some configurations we used ourselves. The previous CodeReview said not to use so many image numbers, so I put all of them here in the configuration.

private Duration distributedLockRetryInterval = Duration.ofSeconds(5); private String module = "master"; private Duration heartBeatTimeOut = Duration.ofSeconds(20); private Duration listenerCheckInterval = Duration.ofSeconds(3); private Duration connectStateCheckInterval = Duration.ofSeconds(2); private int subscribeListenerThreadPoolSize = 1; private int connectionListenerThreadPoolSize = 1;

What should be done to make it better?

Some of these configurations have put into classes. Only the configurations that users must set are retained.

private String clusterName; private String serverAddressList; private String serverAddress; private int serverPort; private String logStorageDir;

At the same time, explanations of the configurations are added in the markdown of the plugin.

ruanwenjun · 2024-08-19T06:44:13Z

dolphinscheduler-dist/release-docs/LICENSE

+    jraft-core 1.3.14: https://mvnrepository.com/artifact/com.alipay.sofa/jraft-core/1.3.14 Apache 2.0
+    jctools-core 2.1.1: https://mvnrepository.com/artifact/org.jctools/jctools-core/2.1.1 Apache 2.0
+    commons-lang 2.6 https://mvnrepository.com/artifact/commons-lang/commons-lang/2.6 Apache 2.0
+    hessian 3.3.6: https://mvnrepository.com/artifact/com.alipay.sofa/hessian/3.3.6 Apache 2.0
+    metrics-core 4.2.11: https://mvnrepository.com/artifact/io.dropwizard.metrics/metrics-core/4.2.11 Apache 2.0
+    affinity 3.1.7: https://mvnrepository.com/artifact/net.openhft/affinity/3.1.7, Apache 2.0
+    disruptor 3.3.7: https://mvnrepository.com/artifact/com.lmax/disruptor/3.3.7, Apache 2.0
+    commons-compress 1.21: https://mvnrepository.com/artifact/org.apache.commons/commons-compress/1.21 Apache 2.0
+    bolt 1.6.4: https://mvnrepository.com/artifact/com.alipay.sofa/bolt/1.6.4, Apache 2.0
+    rocksdbjni 8.8.1: https://mvnrepository.com/artifact/org.rocksdb/rocksdbjni/8.8.1, Apache 2.0
+    jraft-rheakv-core 1.3.14: https://mvnrepository.com/artifact/com.alipay.sofa/jraft-rheakv-core/1.3.14, Apache 2.0
+    sofa-common-tools 1.0.12: https://mvnrepository.com/artifact/com.alipay.sofa.common/sofa-common-tools/1.0.12 Apache 2.0
+    annotations 12.0: https://mvnrepository.com/artifact/com.intellij/annotations/12.0 Apache 2.0
+    protostuff-core 1.7.2: https://mvnrepository.com/artifact/io.protostuff/protostuff-core/1.7.2 Apache 2.0
+    protostuff-api 1.7.2: https://mvnrepository.com/artifact/io.protostuff/protostuff-api/1.7.2 Apache 2.0
+    protostuff-runtime 1.7.2: https://mvnrepository.com/artifact/io.protostuff/protostuff-runtime/1.7.2 Apache 2.0
+    protostuff-collectionschema 1.7.2: https://mvnrepository.com/artifact/io.protostuff/protostuff-collectionschema/1.7.2 Apache 2.0


why we need to import so many dependencies from alipay?

We introduced this direct dependency.

<dependency> <groupId>com.alipay.sofa</groupId> <artifactId>jraft-rheakv-core</artifactId> <version>${jraft.version}</version> </dependency> </dependencies>

These are all transitive dependencies. I'm not sure if these need to be written in the license.

ruanwenjun

If there are no master can this work? We should select the leader from the whole cluster rather than select the leader from master, I didn't see any design related to this PR, you should provide the newly design doc related to this PR.

MatthewAden · 2024-08-19T09:19:44Z

If there are no master can this work? We should select the leader from the whole cluster rather than select the leader from master, I didn't see any design related to this PR, you should provide the newly design doc related to this PR.

Hi, this is an OSPP project. My mentor is @zhuxt2015. The design plan for the raft plugin has been discussed. The design link is Add new registry plugin based on raft. According to the discussed plan, masters will form a raft cluster, and the leader will only be elected among the masters, just like in ZK. For the three master nodes, at least two must be alive to provide services normally.

sonarcloud · 2024-09-13T08:03:25Z

Please retry analysis of this Pull-Request directly on SonarCloud

MatthewAden requested review from ruanwenjun, Gallardot and SbloodyS as code owners July 20, 2024 12:49

github-actions bot added backend test document labels Jul 20, 2024

github-actions bot assigned MatthewAden Jul 20, 2024

SbloodyS added this to the 3.3.0 milestone Jul 20, 2024

SbloodyS added the first time contributor First-time contributor label Jul 20, 2024

SbloodyS requested a review from caishunfeng July 20, 2024 13:03

SbloodyS reviewed Jul 20, 2024

View reviewed changes

SbloodyS added the feature new feature label Jul 20, 2024

MatthewAden force-pushed the feat_register_raft branch from 21f6409 to da9a816 Compare July 20, 2024 13:24

github-actions bot removed the document label Jul 20, 2024

MatthewAden force-pushed the feat_register_raft branch from da9a816 to b477fce Compare July 20, 2024 13:28

github-actions bot added the document label Jul 20, 2024

github-advanced-security bot found potential problems Jul 20, 2024

View reviewed changes

MatthewAden changed the title ~~[Feature-10874][registry]add raft registration plugin~~ [Feature-10874][registry] add raft registration plugin Jul 21, 2024

SbloodyS reviewed Jul 23, 2024

View reviewed changes

MatthewAden force-pushed the feat_register_raft branch 3 times, most recently from 6f299ab to 71de6b2 Compare July 30, 2024 00:13

MatthewAden force-pushed the feat_register_raft branch from 71de6b2 to c638010 Compare August 19, 2024 03:25

SbloodyS requested a review from Radeity August 19, 2024 06:25

ruanwenjun reviewed Aug 19, 2024

View reviewed changes

SbloodyS added the DSIP label Aug 30, 2024

SbloodyS changed the title ~~[Feature-10874][registry] add raft registration plugin~~ [DSIP-9][registry] add raft registration plugin Aug 30, 2024

MatthewAden force-pushed the feat_register_raft branch from c638010 to 9f50e1c Compare September 8, 2024 03:28

MatthewAden added 7 commits September 11, 2024 15:34

[DSIP-9][feature]add raft registration plugin

c854725

Fix magic numbers and Adjust auto-configuration

b763e70

Fix magic numbers

a33a5ce

Add logic to remove ephemeral nodes upon disconnection

8c73f99

Fix the abnormal startup of the raft cluster（apache#10874）

eab6457

Adjust to form a raft cluster with all nodes.（apache#10874）

593dfe0

Remove redundant configurations from the test module.（apache#10874）

d98caa1

MatthewAden force-pushed the feat_register_raft branch from 9f50e1c to d98caa1 Compare September 11, 2024 07:35

Supplementary dependency information.（apache#10874）

68a74d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DSIP-9][registry] add raft registration plugin #16352

[DSIP-9][registry] add raft registration plugin #16352

MatthewAden commented Jul 20, 2024 •

edited

Loading

boring-cyborg bot commented Jul 20, 2024

ruanwenjun Aug 19, 2024

MatthewAden Aug 19, 2024

MatthewAden Sep 8, 2024

ruanwenjun Aug 19, 2024

MatthewAden Aug 19, 2024

ruanwenjun Aug 30, 2024

zhuxt2015 Aug 30, 2024

SbloodyS Aug 30, 2024

zhuxt2015 Aug 30, 2024

MatthewAden Sep 8, 2024

ruanwenjun Aug 19, 2024

MatthewAden Aug 19, 2024

MatthewAden Sep 8, 2024

ruanwenjun Aug 19, 2024

MatthewAden Aug 19, 2024

ruanwenjun left a comment

MatthewAden commented Aug 19, 2024

sonarcloud bot commented Sep 13, 2024

[DSIP-9][registry] add raft registration plugin #16352

Are you sure you want to change the base?

[DSIP-9][registry] add raft registration plugin #16352

Conversation

MatthewAden commented Jul 20, 2024 • edited Loading

Purpose of the pull request

Brief change log

Verify this pull request

Unit Test Results:

Pseudo-Cluster Deployment Status:

Cluster Test

boring-cyborg bot commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanwenjun left a comment

Choose a reason for hiding this comment

MatthewAden commented Aug 19, 2024

sonarcloud bot commented Sep 13, 2024

MatthewAden commented Jul 20, 2024 •

edited

Loading