[Feature][Registry] Support etcd as registry by wjf222 · Pull Request #10981 · apache/dolphinscheduler

wjf222 · 2022-07-15T02:00:32Z

Purpose of the pull request

Close #8975

Brief change log

Add CRUD
Add Subscribe
Add Lock
Add ConnectStateListener

Verify this pull request

This change added tests and can be verified as follows:
Add persistTest to test for CRUD
Add lockTest for lock

TODO add connectStateListener Test

kezhenxu94 · 2022-07-15T02:04:28Z

...d/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistryProperties.java

+    private Long retryDelay=60L;
+    private Long retryMaxDelay=300L;


Consider also using Duration type, and format the codes please.

The time unit for these two parameters is set by the ChronUnitRetry in jetcd. The type of these two parameters is long, but the type of retryMaxDuration is Duration.

You can see the reference of these three parameters in EtcdRegistry.java line 51 to 54.

The time unit for these two parameters is set by the ChronUnitRetry in jetcd. The type of these two parameters is long, but the type of retryMaxDuration is Duration.

You can see the reference of these three parameters in EtcdRegistry.java line 51 to 54.

Hi, you don't have to just copy the type from JETCD client, using Duration as type in our own configurations is for users' convenient, they can just set something like 1s, 500ms, without knowing what's the time unit of the config. Also, users don't care what types we pass into EtcdRegistry.java, let's provide simplicity to users

The time unit for these two parameters is set by the ChronUnitRetry in jetcd. The type of these two parameters is long, but the type of retryMaxDuration is Duration.
You can see the reference of these three parameters in EtcdRegistry.java line 51 to 54.

Hi, you don't have to just copy the type from JETCD client, using Duration as type in our own configurations is for users' convenient, they can just set something like 1s, 500ms, without knowing what's the time unit of the config. Also, users don't care what types we pass into EtcdRegistry.java, let's provide simplicity to users

You are right,I fix this in Modify the type of delay

kezhenxu94 · 2022-07-15T02:04:34Z

...d/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistryProperties.java

+    // RetryPolicy
+    private Long retryDelay=60L;
+    private Long retryMaxDelay=300L;
+    private Duration retryMaxDuration=Duration.ofMillis(1500);;


Suggested change

private Duration retryMaxDuration=Duration.ofMillis(1500);;

private Duration retryMaxDuration=Duration.ofMillis(1500);

kezhenxu94 · 2022-07-15T02:05:11Z

...nscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-etcd/pom.xml

+        <dependency>
+            <groupId>io.etcd</groupId>
+            <artifactId>jetcd-test</artifactId>
+        </dependency>


This should be test scope

kezhenxu94 · 2022-07-15T02:05:33Z

...nscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-etcd/pom.xml

+    <properties>
+        <maven.compiler.source>8</maven.compiler.source>
+        <maven.compiler.target>8</maven.compiler.target>
+    </properties>


Don't set this here

kezhenxu94 · 2022-07-15T02:36:26Z

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

+
+# How to use
+
+If you want to set the registry center as mysql,You need to set the registry properties in master/worker/api's appplication.yml


Suggested change

If you want to set the registry center as mysql,You need to set the registry properties in master/worker/api's appplication.yml

If you want to set the registry center as mysql, you need to set the registry properties in master/worker/api's appplication.yml

...nscheduler-registry/dolphinscheduler-registry-plugins/dolphinscheduler-registry-etcd/pom.xml

kezhenxu94 · 2022-07-15T02:49:39Z

...d/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistryProperties.java

+    private Long retryDelay=60L;
+    private Long retryMaxDelay=300L;


The time unit for these two parameters is set by the ChronUnitRetry in jetcd. The type of these two parameters is long, but the type of retryMaxDuration is Duration.

You can see the reference of these three parameters in EtcdRegistry.java line 51 to 54.

Hi, you don't have to just copy the type from JETCD client, using Duration as type in our own configurations is for users' convenient, they can just set something like 1s, 500ms, without knowing what's the time unit of the config. Also, users don't care what types we pass into EtcdRegistry.java, let's provide simplicity to users

kezhenxu94 · 2022-07-15T02:51:15Z

...gistry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistry.java

+    public void put(String key, String value, boolean deleteOnDisconnect) {
+        try{
+            if(deleteOnDisconnect) {
+                // keep the key by lease, if disconnected, the lease will ,the key will delete


This sentence is not complete...

kezhenxu94 · 2022-07-15T02:53:28Z

.../main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdConnectionStateListener.java

+        try {
+            Field connectField =client.getClass().getDeclaredField("connectManager");
+            if(!connectField.isAccessible()){
+                connectField.setAccessible(true);
+            }
+            Object connection = connectField.get(client);
+            Method channel = connection.getClass().getDeclaredMethod("getChannel");
+            if (!channel.isAccessible()) {
+                channel.setAccessible(true);
+            }
+            return (ManagedChannel) channel.invoke(connection);
+        } catch (Exception e) {
+            throw new RegistryException("Failed to get the etcd client channel", e);
+        }


I don't think this is a good way to implement the connectivity state listener, why do you choose to use reflection instead of some native method of jetcd client?

Ok, I will try to use keepalive in lease client to listen for connection status.

I don't think this is a good way to implement the connectivity state listener, why do you choose to use reflection instead of some native method of jetcd client?

I made the problem complicated. I only need to periodically try to connect to Etcd, and the connection status can be judged by the connection result.
I have removed the reflection related code. Now, i use the client to apply for a lease to determine whether the connection is successful in Using the lease to listen connection state

kezhenxu94 · 2022-07-15T02:57:59Z

Many files are missing license headers

caishunfeng · 2022-07-15T06:22:20Z

Hi @wjf222 please add the doc of the registry together.

wjf222 · 2022-07-15T07:12:17Z

Many files are missing license headers

I fixed this in the Add license header

wjf222 · 2022-07-15T12:57:26Z

Hi @wjf222 please add the doc of the registry together.

I add the doc in [Document] Add readme and comments.Do you need to add other documents here?

wjf222 · 2022-08-14T06:03:38Z

@kezhenxu94 @caishunfeng @ruanwenjun Please help continue to review. Thank you very much

kezhenxu94

Generally looks good to me, please fix conflict

.../main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdConnectionStateListener.java

...gistry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistry.java

…lphinscheduler-registry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistry.java Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>

…lphinscheduler-registry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdConnectionStateListener.java Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>

wjf222 · 2022-08-17T04:55:21Z

Generally looks good to me, please fix conflict

@kezhenxu94 Thanks, I have fixed the conflict

caishunfeng · 2022-08-23T09:58:37Z

@wjf222 Please fix the conflicts.

wjf222 · 2022-08-23T13:51:20Z

Thanks, I have fixed the conflict

@caishunfeng I have fixed the conflict. Test bugs are due to new 3rd party packages introduced in other commits

wjf222 · 2022-08-25T09:05:48Z

@caishunfeng Please pass the review, I have fixed the conflict. Thank you.

wjf222 · 2022-08-29T12:21:43Z

@kezhenxu94 @caishunfeng @ruanwenjun Due to previous conflicts, modifications have been made, and I hope to pass this review again. Thank you very much.

ruanwenjun · 2022-08-30T09:20:12Z

@wjf222 Please resolve the dependency check failure.

sonarqubecloud · 2022-08-31T03:10:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

44.8% Coverage
2.0% Duplication

wjf222 · 2022-08-31T03:12:33Z

@wjf222 Please resolve the dependency check failure.

@ruanwenjun I have fixed the conflict. Thank you.

wjf222 · 2022-09-03T07:00:03Z

@kezhenxu94 @caishunfeng @ruanwenjun I have passed the test.Please apprve the review.Thank you.

wjf222 · 2022-09-05T07:09:41Z

@kezhenxu94 @caishunfeng @ruanwenjun I have passed the test.Please apprve the review.Thank you.

ruanwenjun

LGTM, I will merged this to dev branch, and this feature will be released at 3.1.0, thanks for your contribution. @wjf222

(cherry picked from commit 2eb8b9f)

(cherry picked from commit 2eb8b9f) Co-authored-by: wjf <jffwang@qq.com>

wjf222 added 9 commits June 30, 2022 11:31

Add dependencyes

de09ac4

Add Test and EtcdRegistry Constructor

73c45b5

Add Crud and Pass EtcdRegistryTest

9774bd3

Add acquireLock and releaseLock

e45adbb

Add subscribe

4aa9bcc

Add ConnectStateListener

cf64c9c

fix some bug

63edd4f

Add comment

c806757

Add README.md

3d13bf5

wjf222 requested review from caishunfeng, kezhenxu94 and ruanwenjun as code owners July 15, 2022 02:00

kezhenxu94 reviewed Jul 15, 2022

View reviewed changes

ruanwenjun assigned wjf222 Jul 15, 2022

ruanwenjun added this to the 3.1.0-alpha milestone Jul 15, 2022

ruanwenjun added the gsoc label Jul 15, 2022

github-actions bot added the backend label Jul 15, 2022

Format the pom.xml

112ff37

kezhenxu94 reviewed Jul 15, 2022

View reviewed changes

Add license header

65d0b9d

caishunfeng added the document label Jul 15, 2022

Modify the type of delay

ba5f768

github-actions bot removed the document label Jul 15, 2022

wjf222 added 2 commits July 15, 2022 15:34

Using the lease to listen connection state

d3ecd2a

[Document] Add readme and comments

48dc998

Merge branch 'dev' into wjf-add-etcd-registry

a857a30

kezhenxu94 reviewed Aug 17, 2022

View reviewed changes

.../main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdConnectionStateListener.java Outdated Show resolved Hide resolved

...gistry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistry.java Outdated Show resolved Hide resolved

wjf222 and others added 3 commits August 16, 2022 19:36

Merge branch 'dev' into wjf-add-etcd-registry

9874510

Update dolphinscheduler-registry/dolphinscheduler-registry-plugins/do…

a8ea404

…lphinscheduler-registry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdRegistry.java Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>

Update dolphinscheduler-registry/dolphinscheduler-registry-plugins/do…

d2063d6

…lphinscheduler-registry-etcd/src/main/java/org/apache/dolphinscheduler/plugin/registry/etcd/EtcdConnectionStateListener.java Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>

kezhenxu94 previously approved these changes Aug 17, 2022

View reviewed changes

Merge branch 'dev' into wjf-add-etcd-registry

e792ea7

wjf222 dismissed kezhenxu94’s stale review via e792ea7 August 23, 2022 11:36

wjf222 added 2 commits August 30, 2022 19:01

Merge branch 'apache:dev' into wjf-add-etcd-registry

123bc4a

fix checkstyle

290c15c

wjf222 force-pushed the wjf-add-etcd-registry branch from 24227e4 to 290c15c Compare August 31, 2022 02:27

ruanwenjun approved these changes Sep 6, 2022

View reviewed changes

ruanwenjun merged commit 2eb8b9f into apache:dev Sep 6, 2022

fengjian1129 pushed a commit to fengjian1129/dolphinscheduler that referenced this pull request Sep 13, 2022

[Feature][Registry] Support etcd as registry (apache#10981)

1f7294e

ruanwenjun pushed a commit to ruanwenjun/dolphinscheduler that referenced this pull request Sep 16, 2022

[Feature][Registry] Support etcd as registry (apache#10981)

9aba00b

(cherry picked from commit 2eb8b9f)

caishunfeng pushed a commit that referenced this pull request Sep 17, 2022

[Feature][Registry] Support etcd as registry (#10981) (#11997)

1ea2e84

(cherry picked from commit 2eb8b9f) Co-authored-by: wjf <jffwang@qq.com>

caishunfeng mentioned this pull request Sep 17, 2022

[Cherry-pick-3.1.0][Feature][Registry] Support etcd as registry (#10981) #11997

Merged

caishunfeng added the release cherry-pick Mark this issue/PR had cherry-pick for release version label Sep 17, 2022

fengjian1129 pushed a commit to fengjian1129/dolphinscheduler that referenced this pull request Sep 29, 2022

[Feature][Registry] Support etcd as registry (apache#10981)

fa9f4eb

xdu-chenrj pushed a commit to xdu-chenrj/dolphinscheduler that referenced this pull request Oct 13, 2022

[Feature][Registry] Support etcd as registry (apache#10981)

57f5ff0

		private Long retryDelay=60L;
		private Long retryMaxDelay=300L;

	private Duration retryMaxDuration=Duration.ofMillis(1500);;
	private Duration retryMaxDuration=Duration.ofMillis(1500);


		# How to use

		If you want to set the registry center as mysql,You need to set the registry properties in master/worker/api's appplication.yml

Conversation

wjf222 commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of the pull request

Brief change log

Verify this pull request

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!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kezhenxu94 commented Jul 15, 2022

Uh oh!

caishunfeng commented Jul 15, 2022

Uh oh!

wjf222 commented Jul 15, 2022

Uh oh!

wjf222 commented Jul 15, 2022

Uh oh!

wjf222 commented Aug 14, 2022

Uh oh!

kezhenxu94 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wjf222 commented Aug 17, 2022

Uh oh!

caishunfeng commented Aug 23, 2022

Uh oh!

wjf222 commented Aug 23, 2022

Uh oh!

wjf222 commented Aug 25, 2022

Uh oh!

wjf222 commented Aug 29, 2022

Uh oh!

ruanwenjun commented Aug 30, 2022

Uh oh!

sonarqubecloud bot commented Aug 31, 2022

Uh oh!

wjf222 commented Aug 31, 2022

Uh oh!

wjf222 commented Sep 3, 2022

Uh oh!

wjf222 commented Sep 5, 2022

Uh oh!

ruanwenjun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wjf222 commented Jul 15, 2022 •

edited

Loading