[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

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

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

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

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.

We should remove all dependencies from alipay.

We should remove all dependencies from alipay.

+1

The raft plugin depends on alipay jraft's rheakv, and other raft implementations do not have this kv store, if can't rely on alipay's jar, then this pr can't be continued, do you want to terminate this pr?

We should remove all dependencies from alipay.

Why not accept Alipay's dependency？

The raft plugin depends on alipay jraft's rheakv, and other raft implementations do not have this kv store, if can't rely on alipay's jar, then this pr can't be continued, do you want to terminate this pr?

Open source projects are submitted to PR voluntarily and free of charge. Please notice that not all PR will be accepted and must be merged.

Why not accept Alipay's dependency？

Registry is the core component of DS. So that it must be robust and maintainable. The alipay's jraft-rheakv-core dependency obviously do not have these conditions and it has a lot of CVE. After using this dependency, every time you fix a problem and CVE in the future, you need to rely on its upgrade, and there may be incompatible updates during the upgrade process. This is unacceptable for the core components.

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

SbloodyS · 2024-09-25T12:59:13Z

Please add mysql and postgresql with raft registry cluster test in CI. You can refer to https://github.com/apache/dolphinscheduler/tree/dev/.github/workflows/cluster-test

ruanwenjun · 2024-09-29T05:41:49Z

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.

If the cluster does have master, then the cluster will not work? This is unacceptable.

zhuxt2015 · 2024-09-29T06:00:54Z

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.

If the cluster does have master, then the cluster will not work? This is unacceptable.

Just like zk plugin, if the zk cluster goes down, the ds cluster will also go down.

ruanwenjun · 2024-09-29T07:05:11Z

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.

If the cluster does have master, then the cluster will not work? This is unacceptable.

Just like zk plugin, if the zk cluster goes down, the ds cluster will also go down.

If so, big -1 for this plugin, this doesn't help with SLA, I can not find any reason why we wouldn't use jdbc registry plugin instead of this plugin.

ruanwenjun · 2024-09-29T07:07:32Z

...uler-registry-api/src/main/java/org/apache/dolphinscheduler/registry/api/RegistryClient.java

+    @PostConstruct
+    public void initializeRegistryPaths() {
+        try {
+            if (!registry.exists(RegistryNodeType.MASTER.getRegistryPath())) {
+                registry.put(RegistryNodeType.MASTER.getRegistryPath(), EMPTY, false);
+            }
+            if (!registry.exists(RegistryNodeType.WORKER.getRegistryPath())) {
+                registry.put(RegistryNodeType.WORKER.getRegistryPath(), EMPTY, false);
+            }
+            if (!registry.exists(RegistryNodeType.ALERT_SERVER.getRegistryPath())) {
+                registry.put(RegistryNodeType.ALERT_SERVER.getRegistryPath(), EMPTY, false);
+            }
+        } catch (Exception e) {
+            log.error("Failed to initialize registry paths", e);
        }


Please don't eat the exception here, why you change the code? Just for happy?

ruanwenjun · 2024-09-29T07:09:34Z

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

+            while (System.currentTimeMillis() < endTimeMillis) {
+                if (raftRegistryClient.isConnectivity()) {
+                    return;
+                }
+            }


This will cause the CPU bump, and process killed.

ruanwenjun · 2024-09-29T07:12:17Z

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

+    public Collection<String> getRegistryDataChildren(String key) {
+        String basePath = null;
+        if (key.startsWith(RegistryNodeType.MASTER.getRegistryPath())) {
+            basePath = RegistryNodeType.MASTER.getRegistryPath();
+        } else if (key.startsWith(RegistryNodeType.WORKER.getRegistryPath())) {
+            basePath = RegistryNodeType.WORKER.getRegistryPath();
+        } else if (key.startsWith(RegistryNodeType.ALERT_SERVER.getRegistryPath())) {
+            basePath = RegistryNodeType.ALERT_SERVER.getRegistryPath();
+        } else {
+            throw new UnsupportedOperationException("unsupported get registry data children by key:" + key);
+        }
+        List<KVEntry> kvEntries = rheaKvStore.bScan(basePath + Constants.SINGLE_SLASH,
+                basePath + Constants.SINGLE_SLASH + Constants.RAFT_END_KEY);
+        return getRegistryList(kvEntries);


It's strange, the plugin layer needs to know the business logic.

SbloodyS · 2024-09-29T13:07:04Z

If so, big -1 for this plugin, this doesn't help with SLA, I can not find any reason why we wouldn't use jdbc registry plugin instead > of this plugin.

+1, High availability and stability are the first things we should ensure. Any new function should not be based on lowering the standards of these two.

zhuxt2015 · 2024-09-29T13:15:16Z

If so, big -1 for this plugin, this doesn't help with SLA, I can not find any reason why we wouldn't use jdbc registry plugin instead > of this plugin.

+1, High availability and stability are the first things we should ensure. Any new function should not be based on lowering the standards of these two.

This plugin does not reduce the stability and availability of the ds cluster, and will maintain the same impact on the cluster as other plugins.

SbloodyS · 2024-09-29T13:23:48Z

This plugin does not reduce the stability and availability of the ds cluster, and will maintain the same impact on the cluster as other plugins.

If the cluster does have master, then the cluster will not work.

This is against zk and jdbc registry since neither of these two types will cause the worker server and alert server to fail to operate normally because of the downtime of the master server.

zhuxt2015 · 2024-09-29T13:40:17Z

This plugin does not reduce the stability and availability of the ds cluster, and will maintain the same impact on the cluster as other plugins.

If the cluster does have master, then the cluster will not work.

This is against zk and jdbc registry since neither of these two types will cause the worker server and alert server to fail to operate normally because of the downtime of the master server.

Raft plugin won‘t do that either.

SbloodyS · 2024-09-30T01:01:51Z

Raft plugin won‘t do that either.

Based on this PR, this problem exists. Why do you say this?

zhuxt2015 · 2024-09-30T02:29:24Z

Raft plugin won‘t do that either.

Based on this PR, this problem exists. Why do you say this?

I’m sorry that I didn‘t explain it clearly. I mean we will fix this problem.

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 2 times, most recently from 9f50e1c to d98caa1 Compare September 11, 2024 07:35

MatthewAden added 8 commits September 25, 2024 18:54

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

2899204

Fix magic numbers and Adjust auto-configuration

cdcb331

Fix magic numbers

ec1dfb5

Add logic to remove ephemeral nodes upon disconnection

7ad3133

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

c619286

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

e8d6b8f

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

e47f72a

Supplementary dependency information.（apache#10874）

2fee2d1

MatthewAden force-pushed the feat_register_raft branch from 68a74d0 to 2fee2d1 Compare September 25, 2024 10:54

ruanwenjun reviewed Sep 29, 2024

View reviewed changes

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

ruanwenjun Sep 29, 2024

SbloodyS Sep 29, 2024

zhuxt2015 Oct 16, 2024

zhuxt2015 Oct 16, 2024

SbloodyS Oct 18, 2024

ruanwenjun left a comment

MatthewAden commented Aug 19, 2024

sonarcloud bot commented Sep 13, 2024

SbloodyS commented Sep 25, 2024

ruanwenjun commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

ruanwenjun commented Sep 29, 2024

ruanwenjun Sep 29, 2024

ruanwenjun Sep 29, 2024

ruanwenjun Sep 29, 2024

SbloodyS commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

SbloodyS commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

SbloodyS commented Sep 30, 2024

zhuxt2015 commented Sep 30, 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

ruanwenjun left a comment

Choose a reason for hiding this comment

MatthewAden commented Aug 19, 2024

sonarcloud bot commented Sep 13, 2024

SbloodyS commented Sep 25, 2024

ruanwenjun commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

ruanwenjun commented Sep 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SbloodyS commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

SbloodyS commented Sep 29, 2024

zhuxt2015 commented Sep 29, 2024

SbloodyS commented Sep 30, 2024

zhuxt2015 commented Sep 30, 2024

MatthewAden commented Jul 20, 2024 •

edited

Loading