Skip to content

[Feature-3134]Refactor to use a shared singleton Curator Zookeeper Client#3244

Merged
qiaozhanwei merged 11 commits intoapache:devfrom
tswstarplanet:singleton_curator_client_2
Jul 23, 2020
Merged

[Feature-3134]Refactor to use a shared singleton Curator Zookeeper Client#3244
qiaozhanwei merged 11 commits intoapache:devfrom
tswstarplanet:singleton_curator_client_2

Conversation

@tswstarplanet
Copy link
Copy Markdown
Contributor

@tswstarplanet tswstarplanet commented Jul 19, 2020 •
edited
Loading

Tips

What is the purpose of the pull request

fix #3134

Brief change log

(for example:)

  • Add a CuratorZookeeperClient class to hold a shared CuratorFramework instance.

Verify this pull request

(Please pick either of the following options)

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added dolphinscheduler-dao tests for end-to-end.
  • Added CronUtilsTest to verify the change.
  • Manually verified the change by testing locally.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 20, 2020

Codecov Report

Merging #3244 into dev will increase coverage by 0.23%.
The diff coverage is 42.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #3244      +/-   ##
============================================
+ Coverage     34.15%   34.39%   +0.23%     
- Complexity     2422     2446      +24     
============================================
  Files           443      444       +1     
  Lines         20637    20634       -3     
  Branches       2531     2527       -4     
============================================
+ Hits           7049     7097      +48     
+ Misses        12926    12870      -56     
- Partials        662      667       +5
Impacted Files Coverage Δ Complexity Δ
...e/dolphinscheduler/api/utils/ZookeeperMonitor.java 6.45% <0.00%> (+0.20%) 2.00 <0.00> (ø)
...heduler/server/master/registry/MasterRegistry.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
.../dolphinscheduler/service/zk/AbstractZKClient.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...nscheduler/service/zk/ZookeeperCachedOperator.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...dolphinscheduler/service/zk/ZookeeperOperator.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...inscheduler/service/zk/CuratorZookeeperClient.java 64.86% <64.86%> (ø) 8.00 <8.00> (?)
...he/dolphinscheduler/api/service/LoggerService.java 82.60% <0.00%> (-8.70%) 9.00% <0.00%> (-1.00%)
...e/dolphinscheduler/service/zk/ZookeeperConfig.java 92.85% <0.00%> (+92.85%) 17.00% <0.00%> (+17.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b54c76...e67e2e0. Read the comment docs.

yangyichao-mango
yangyichao-mango reviewed Jul 21, 2020
View reviewed changes
...ler-service/src/main/java/org/apache/dolphinscheduler/service/zk/CuratorZookeeperClient.java
public void initStateLister() {
checkNotNull(zkClient);

zkClient.getConnectionStateListenable().addListener((client, newState) -> {
Copy link
Copy Markdown
Contributor

@yangyichao-mango yangyichao-mango Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Because remove the master zk address log from MasterRegistry, please add the master zk address to log, not the zk server info.

因为我们是从masterregistry中删除了master zk节点的日志，想要添加到CuratorZkCLient中。所以CuratorZkCLient日志信息应该是master zk节点的日志信息，而不是zk地址信息。

If you have any question or suggestion, please put forward.

Copy link
Copy Markdown
Contributor

@yangyichao-mango yangyichao-mango Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

你好，原来的日志打印的也是zookeeper的地址。另外，这种连接状态的监控，应该是用一个listener监听就可以了，重复的监听是没有意义的。

image

Copy link
Copy Markdown
Contributor Author

@tswstarplanet tswstarplanet Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

又看了一下，这里不只是日志的问题，而是原来代码里在Reconnected的时候，有个保存临时节点的操作。那可能确实不能简单地删掉，还要再分析下

Copy link
Copy Markdown
Contributor

@yangyichao-mango yangyichao-mango Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

又看了一下，这里不是日志的问题，而是原来代码里在Reconnected的时候，有个保存临时节点的操作。那可能确实不能简单地删掉，还要再分析下

I agree with you~

Copy link
Copy Markdown
Contributor Author

@tswstarplanet tswstarplanet Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已经把对这个类的修改恢复了

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jul 21, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 9 Code Smells

50.0% 50.0% Coverage
0.0% 0.0% Duplication

yangyichao-mango
yangyichao-mango approved these changes Jul 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yangyichao-mango yangyichao-mango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

qiaozhanwei
qiaozhanwei approved these changes Jul 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@qiaozhanwei qiaozhanwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@qiaozhanwei qiaozhanwei merged commit 5b7efd2 into apache:dev Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qiaozhanwei qiaozhanwei qiaozhanwei approved these changes

@yangyichao-mango yangyichao-mango yangyichao-mango approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature]Refactor curator zookeeper client to be singleton

4 participants

@tswstarplanet @codecov-commenter @qiaozhanwei @yangyichao-mango