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

Add ZooKeeper data source for Sentinel #33

Merged
merged 10 commits into from
Aug 5, 2018
Merged

Conversation

guonanjun
Copy link
Contributor

@guonanjun guonanjun commented Aug 4, 2018

Describe what this PR does / why we need it

增加了zookeeper的数据源和测试demo

Resolves #29

Describe how you did it

参考nacos数据源,保持和nacos数据源风格一致,通过zkclient实现zookeeper的发布订阅功能,可以在规则变更的时候主动推送规则。

Describe how to verify it

sentinel-demo-zookeeper-datasource里面的ZookeeperDataSourceDemo是模拟微服务启动的时候注册数据源的场景,ZookeeperConfigSender是模拟Sentinel控制台在规则变更时,主动推送规则给微服务的场景。

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Aug 4, 2018

Codecov Report

Merging #33 into master will decrease coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #33      +/-   ##
============================================
- Coverage     46.54%   46.37%   -0.17%     
+ Complexity      544      541       -3     
============================================
  Files           105      105              
  Lines          3657     3657              
  Branches        519      519              
============================================
- Hits           1702     1696       -6     
- Misses         1751     1754       +3     
- Partials        204      207       +3
Impacted Files Coverage Δ Complexity Δ
...ntinel/slots/statistic/metric/WindowLeapArray.java 70.96% <0%> (-9.68%) 7% <0%> (-1%)
...a/com/alibaba/csp/sentinel/node/StatisticNode.java 61.4% <0%> (-5.27%) 15% <0%> (-2%)

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 b2a3ef2...1a8bd28. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Aug 4, 2018
Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

Thanks for contribution! Maybe using Curator is better? It's considered that Curator is better supported than zkclient.

@sczyh30 sczyh30 changed the title 增加zookeeper数据源 Add ZooKeeper data source for Sentinel Aug 4, 2018
Copy link
Contributor Author

@guonanjun guonanjun left a comment

Choose a reason for hiding this comment

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

将zkclient替换为curator

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

Good. Maybe a basic test case is required since Curator has a local test server for testing (curator-test module). Nacos integration does not have test cases because Nacos does not provide this kind of mechanism currently.

private CuratorFramework zkClient = null;
private NodeCache nodeCache = null;

public ZookeeperDataSource(final String serverAddr, final String groupId, final String dataId,
Copy link
Member

Choose a reason for hiding this comment

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

The groupId and dataId are concepts in Nacos. In ZooKeeper a path is enough (should check to make sure it's started with /).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

为了保持和nacos风格统一,groupId和dataId都不用以 / 开头,数据源里面会自动加。
String path = "/" + this.groupId + "/" + this.dataId;
Stat stat = this.zkClient.checkExists().forPath(path);

@sczyh30
Copy link
Member

sczyh30 commented Aug 5, 2018

我感觉这里需要加一个只需传入 path 的构造函数:

public ZookeeperDataSource(final String serverAddr, final String path, ConfigParser<String, T> parser)

然后 groupIddataId 版本的复用即可:

public ZookeeperDataSource(final String serverAddr, final String groupId, final String dataId, ConfigParser<String, T> parser) {
  this(serverAddr, getPath(groupId, dataId), parser);
}

因为对 ZooKeeper 用户来说,没有 groupIddataId 的概念。即使为了平滑迁移到 Nacos 而保留这个版本的构造函数,也需要给用户最原始的方式指定 path。对迁移到 Nacos 来说也没有什么成本,封装一个 getPath 函数即可。

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

@sczyh30 sczyh30 merged commit 3395412 into alibaba:master Aug 5, 2018
@sczyh30 sczyh30 removed the to-review To review label Aug 5, 2018
sczyh30 pushed a commit that referenced this pull request Aug 8, 2018
Arlmls pushed a commit to Arlmls/Sentinel that referenced this pull request Jan 8, 2019
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