Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor zk client and zk config module #1537

Merged
merged 3 commits into from
Dec 25, 2019
Merged

refactor zk client and zk config module #1537

merged 3 commits into from
Dec 25, 2019

Conversation

clay4444
Copy link
Contributor

What is the purpose of the pull request

the main purpose of this pr is to refactor the dependency on zk, including zk config dependency & zk client dependency.

Brief change log

In the process of refactoring, I have tried to be compatible with the previous code as much as possible, but still involves more file changes, so here are some important points that need to be explained for the convenience of code review

  1. the main purpose of this refactor is to leave the zk configuration and zk client to the spring container for management, but in order to be compatible with the implementation of TaskQueueZkImpl (all references to this class are static methods), so conf file: zookeeper.properties is still retained.
  2. As mentioned above, all methods that reference the TaskQueueZkImpl class are static methods. If refactor it, more files will be involved, and the code review will be more difficult, so the temporary implementation here is TaskQueueZkImpl created an additional zk client. This is where I think there may be problems with this refactoring, but I have not thought of a good way for the time being, welcome everyone to point out
  3. ZookeeperOperator is the top-level implementation class. It is responsible for creating the zk client and encapsulates all basic operations on zk. ZookeeperCachedOperator is mainly responsible for listening to zk. All listeners are created and closed here.
  4. ZKMasterClient and ZKWorkerClient are implemented by inheriting ZookeeperOperator, and use the upper layer to provide basic operations to encapsulate the business logic they need, but for the convenience of code review, the business logic code has not been modified. If the refactoring scheme passes, I will completely modify it;

  1. 这次重构的主要目的是把zk的配置和zk的客户端交给spring容器来管理,但是为了兼容TaskQueueZkImpl的实现(所有引用这个类的地方都是静态方法),所以仍然保留了zookeeper.properties这个配置文件
  2. 就像上面提到的,所有引用TaskQueueZkImpl 这个类的方法都是静态方法,如果对它进行重构,会涉及更多的文件,会对代码review带来更大的困难,所以这里暂时的实现是TaskQueueZkImpl创建了一个额外的zk客户端,这是我觉得这次重构可能有问题的地方,但是暂时没有想到好的办法,欢迎大家指点
  3. ZookeeperOperator 是最上层的实现类,它负责创建zk客户端,并封装了所有对zk的基本操作,ZookeeperCachedOperator 主要负责对zk的监听,所有监听器都在这里统一创建和关闭,
  4. ZKMasterClient 和 ZKWorkerClient 通过继承ZookeeperOperator实现,用上层提供基础操作来封装各自需要的业务逻辑,但是为了方便代码review,业务逻辑代码并没有修改,如果重构方案通过,我会彻底修改完成;

Verify this pull request

This pull request is code refactor and whether the refactoring solution is feasible still needs everyone to point out, If the final plan is passed, I will continue to add all the UT involved in this pr

@codecov-io
Copy link

codecov-io commented Dec 21, 2019

Codecov Report

Merging #1537 into dev will decrease coverage by 0.15%.
The diff coverage is 8.87%.

Impacted file tree graph

@@           Coverage Diff            @@
##             dev   #1537      +/-   ##
========================================
- Coverage   7.42%   7.27%   -0.16%     
========================================
  Files        272     278       +6     
  Lines      13634   13738     +104     
  Branches    2169    2196      +27     
========================================
- Hits        1012     999      -13     
- Misses     12555   12674     +119     
+ Partials      67      65       -2
Impacted Files Coverage Δ
...e/dolphinscheduler/server/worker/WorkerServer.java 0% <ø> (ø) ⬆️
.../org/apache/dolphinscheduler/common/Constants.java 0% <ø> (ø) ⬆️
...e/dolphinscheduler/common/zk/AbstractZKClient.java 0% <0%> (-15.24%) ⬇️
.../dolphinscheduler/common/zk/ZookeeperOperator.java 0% <0%> (ø)
...e/dolphinscheduler/api/utils/ZookeeperMonitor.java 0% <0%> (ø) ⬆️
...che/dolphinscheduler/server/zk/ZKWorkerClient.java 0% <0%> (ø) ⬆️
...inscheduler/common/zk/ZookeeperCachedOperator.java 0% <0%> (ø)
...e/dolphinscheduler/server/master/MasterServer.java 0% <0%> (ø) ⬆️
...e/dolphinscheduler/api/service/MonitorService.java 0% <0%> (ø) ⬆️
...che/dolphinscheduler/server/zk/ZKMasterClient.java 0% <0%> (ø) ⬆️
... and 13 more

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 d52a83b...8133768. Read the comment docs.

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

TaskQueueZkImpl is not nice.
Should not have initZkClient to init zk.
System should use the same client in ZookeeperOperator

@clay4444
Copy link
Contributor Author

TaskQueueZkImpl is not nice.
Should not have initZkClient to init zk.
System should use the same client in ZookeeperOperator

yeah~, I have find a better way to handle this, I will implement it later

* @param cachePath zk path
* @param listener operator
*/
public void registerListener(final String cachePath, final TreeCacheListener listener) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not a good way here
listen a base path is why AbstractListener borns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I get it.

Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants