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

ZookeeperDataSource might not be initialized well unless the ZooKeeper server is active(#588) #597

Merged
merged 1 commit into from
Mar 25, 2019

Conversation

zhousiliang163
Copy link
Contributor

Describe what this PR does / why we need it

When the ZookeeperDataSource data source is initialized, if the zk server cannot be used or the connection between the client and the zk server is timed out due to network reasons, the monitoring of the data of the znode node may fail

Does this pull request fix one issue?

Fixes #588

Describe how you did it

NodeCache类本身已经实现了对zk的连接和重连事件进行监听,源码如下:

 private ConnectionStateListener connectionStateListener = new ConnectionStateListener()
    {
        @Override
        public void stateChanged(CuratorFramework client, ConnectionState newState)
        {
            if ( (newState == ConnectionState.CONNECTED) || (newState == ConnectionState.RECONNECTED) )
            {
                if ( isConnected.compareAndSet(false, true) )
                {
                    try
                    {
                        reset();
                    }
                    catch ( Exception e )
                    {
                        ThreadUtils.checkInterrupted(e);
                        log.error("Trying to reset after reconnection", e);
                    }
                }
            }
            else
            {
                isConnected.set(false);
            }
        }
    };

reset()方法会设置nodeCache的data属性,所以用户可以通过nodeCache.getCurrentData()方法获取。

Describe how to verify it

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Mar 20, 2019

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #597 into master will increase coverage by 0.58%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #597      +/-   ##
============================================
+ Coverage     39.28%   39.87%   +0.58%     
- Complexity     1229     1246      +17     
============================================
  Files           275      275              
  Lines          8702     8699       -3     
  Branches       1163     1161       -2     
============================================
+ Hits           3419     3469      +50     
+ Misses         4846     4784      -62     
- Partials        437      446       +9
Impacted Files Coverage Δ Complexity Δ
...inel/datasource/zookeeper/ZookeeperDataSource.java 61.11% <81.81%> (-0.23%) 9 <0> (+1)
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java 95.23% <0%> (-4.77%) 7% <0%> (-1%)
...l/cluster/server/connection/ConnectionManager.java 76% <0%> (+2%) 11% <0%> (+1%) ⬆️
...a/csp/sentinel/slots/statistic/base/LeapArray.java 70.58% <0%> (+2.94%) 34% <0%> (+1%) ⬆️
...m/alibaba/csp/sentinel/log/DateFileLogHandler.java 57.57% <0%> (+3.03%) 7% <0%> (+1%) ⬆️
...a/csp/sentinel/slots/statistic/base/LongAdder.java 34.04% <0%> (+17.02%) 11% <0%> (+7%) ⬆️
...a/csp/sentinel/slots/statistic/base/Striped64.java 66.66% <0%> (+40.62%) 12% <0%> (+7%) ⬆️

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 9051a6b...23589ec. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Mar 21, 2019
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.

Could you please explain your changes?

@zhousiliang163
Copy link
Contributor Author

Could you please explain your changes?

        Stat stat = this.zkClient.checkExists().forPath(this.path);
          if (stat == null) {
              this.zkClient.create().creatingParentContainersIfNeeded().withMode(CreateMode.PERSISTENT).forPath(this.path, null);
          }

把上面的代码删除有两个原因:
一、ZookeeperDataSource是对数据源的监听,没有必要去判断是否存在这个path,并且可能去创建这个path.
二、当ZookeeperDataSource初始化时,如果zk 服务器异常或者网络问题,会导致对数据源的监听失败。

把代码:

  byte[] data = this.zkClient.getData().forPath(this.path);
     if (data != null) {
         return new String(data);

修改为:


  ChildData childData = nodeCache.getCurrentData();
        if (null != childData && childData.getData() != null) {

            configInfo = new String(childData.getData());
        }

当ZookeeperDataSource初始化时,如果zk 服务器异常或者网络问题this.zkClient.getData().forPath(this.path)会执行得特别慢,最终会报错。
nodeCache.getCurrentData()可以代替它

@zhousiliang163
Copy link
Contributor Author

Could you please explain your changes?

最终实现初始化时对zk数据源的监听与zk服务器、网络等是否正常无关

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 12c67d4 into alibaba:master Mar 25, 2019
@sczyh30
Copy link
Member

sczyh30 commented Mar 25, 2019

Thanks for contributing!

@sczyh30 sczyh30 removed the to-review To review label Mar 25, 2019
@sczyh30 sczyh30 added this to the 1.5.1 milestone Mar 25, 2019
blindpirate pushed a commit to blindpirate/Sentinel that referenced this pull request Apr 10, 2019
CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
[ISSUE alibaba#525] Support the message track. Merge to branch msg_track
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.

ZookeeperDataSource might not be initialized well unless the ZooKeeper server is active
4 participants