You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@dubbo.apache.org by "cvictory (GitHub)" <gi...@apache.org> on 2019/03/07 03:05:40 UTC

[GitHub] [incubator-dubbo] cvictory opened pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

The purpose of this pull request:
1. In configcenter module, the DynamicConfiguration is using Curator Api directly.  Add some api and feature into zookeeperClient so that it can satisfy DynamicConfiguration.  
2. Move curator  to  zookeeperClient in in ZookeeperDynamicConfiguration.java
3. Make zookeeper can reuse the connection.

The changes:
* And some api and function in remoting-zookeeper
* Make ZookeeperDynamicConfiguration.java use zookeeper client api.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] cvictory commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "cvictory (GitHub)" <gi...@apache.org>.

And remove zkclient from dubbo:  https://github.com/apache/incubator-dubbo/issues/3569

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=h1) Report
> Merging [#3603](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/c65b589fc2b5cebed0138386f8e62e3c2528691e?src=pr&el=desc) will **increase** coverage by `0.31%`.
> The diff coverage is `60.86%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##             master   #3603      +/-   ##
===========================================
+ Coverage     63.99%   64.3%   +0.31%     
  Complexity       71      71              
===========================================
  Files           700     699       -1     
  Lines         30684   31389     +705     
  Branches       4973    5203     +230     
===========================================
+ Hits          19636   20186     +550     
- Misses         8788    8878      +90     
- Partials       2260    2325      +65
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...g/apache/dubbo/configcenter/ConfigChangeEvent.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci1hcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9Db25maWdDaGFuZ2VFdmVudC5qYXZh) | `72.72% <ø> (+2.72%)` | `0 <0> (ø)` | :arrow_down: |
| [...ookeeper/ZookeeperDynamicConfigurationFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9ab29rZWVwZXJEeW5hbWljQ29uZmlndXJhdGlvbkZhY3RvcnkuamF2YQ==) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [.../configcenter/support/zookeeper/CacheListener.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9DYWNoZUxpc3RlbmVyLmphdmE=) | `62.16% <44.44%> (-3.7%)` | `0 <0> (ø)` | |
| [...ing/zookeeper/support/AbstractZookeeperClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvc3VwcG9ydC9BYnN0cmFjdFpvb2tlZXBlckNsaWVudC5qYXZh) | `70.37% <47.36%> (-7.05%)` | `0 <0> (ø)` | |
| [...org/apache/dubbo/remoting/zookeeper/EventType.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvRXZlbnRUeXBlLmphdmE=) | `61.9% <61.9%> (ø)` | `0 <0> (?)` | |
| [...ting/zookeeper/curator/CuratorZookeeperClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvY3VyYXRvci9DdXJhdG9yWm9va2VlcGVyQ2xpZW50LmphdmE=) | `64.1% <65.71%> (-3.61%)` | `0 <0> (ø)` | |
| [...pport/zookeeper/ZookeeperDynamicConfiguration.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9ab29rZWVwZXJEeW5hbWljQ29uZmlndXJhdGlvbi5qYXZh) | `83.33% <71.42%> (+10.6%)` | `0 <0> (ø)` | :arrow_down: |
| [...le/src/main/java/com/alibaba/dubbo/rpc/Filter.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29tcGF0aWJsZS9zcmMvbWFpbi9qYXZhL2NvbS9hbGliYWJhL2R1YmJvL3JwYy9GaWx0ZXIuamF2YQ==) | `0% <0%> (-100%)` | `0% <0%> (ø)` | |
| [...e/src/main/java/com/alibaba/dubbo/rpc/Invoker.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29tcGF0aWJsZS9zcmMvbWFpbi9qYXZhL2NvbS9hbGliYWJhL2R1YmJvL3JwYy9JbnZva2VyLmphdmE=) | `0% <0%> (-36.37%)` | `0% <0%> (ø)` | |
| [...rc/main/java/com/alibaba/dubbo/rpc/Invocation.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29tcGF0aWJsZS9zcmMvbWFpbi9qYXZhL2NvbS9hbGliYWJhL2R1YmJvL3JwYy9JbnZvY2F0aW9uLmphdmE=) | `33.33% <0%> (-16.67%)` | `0% <0%> (ø)` | |
| ... and [26 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=footer). Last update [c65b589...3ff8235](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

No necessary to keep author info.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Ok, I got it, it's guaranteed by the number of ZookeeperDynamicConfiguration instance.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Will we have too much `TreeCache` instance created? One instance should be enough.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj closed pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

[ pull request closed by chickenlj ]

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

If we decide to remove `zkclient` and rely on `curator` directly, then I think `ZookeeperTransport ` and `ZookeeperClient` is also no longer needed, we can use Curator bounded classes directly.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] ralf0131 commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "ralf0131 (GitHub)" <gi...@apache.org>.

Please resolve the conflict first.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

https://github.com/apache/incubator-dubbo/blob/096d1dae2adc86be71076a53c1ded690cc5b4997/dubbo-registry/dubbo-registry-zookeeper/src/main/java/org/apache/dubbo/registry/zookeeper/ZookeeperRegistryFactory.java#L42

There's no need to pass `ZookeeperTransporter` into `ZookeeperRegistryFactory` and `ZookeeperDynamicConfigurationFactory`, we can build Curator client inside `ZookeeperRegistry or `ZookeeperDynamicConfiguration`？

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Since `CuratorWatcherImpl` implements both TreeCacheListener and CuraorWatcher, can we use CuratorWatcherImpl directly?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] codecov-io commented on issue #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "codecov-io (GitHub)" <gi...@apache.org>.

# [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=h1) Report
> Merging [#3603](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-dubbo/commit/c65b589fc2b5cebed0138386f8e62e3c2528691e?src=pr&el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `62.83%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/graphs/tree.svg?width=650&token=VnEIkiFQT0&height=150&src=pr)](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master    #3603      +/-   ##
============================================
- Coverage     63.99%   63.88%   -0.12%     
  Complexity       71       71              
============================================
  Files           700      698       -2     
  Lines         30684    30618      -66     
  Branches       4973     4972       -1     
============================================
- Hits          19636    19560      -76     
- Misses         8788     8797       +9     
- Partials       2260     2261       +1
```


| [Impacted Files](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...ookeeper/ZookeeperDynamicConfigurationFactory.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9ab29rZWVwZXJEeW5hbWljQ29uZmlndXJhdGlvbkZhY3RvcnkuamF2YQ==) | `100% <100%> (ø)` | `0 <0> (ø)` | :arrow_down: |
| [...pport/zookeeper/ZookeeperDynamicConfiguration.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9ab29rZWVwZXJEeW5hbWljQ29uZmlndXJhdGlvbi5qYXZh) | `90% <100%> (+17.27%)` | `0 <0> (ø)` | :arrow_down: |
| [...ing/zookeeper/support/AbstractZookeeperClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvc3VwcG9ydC9BYnN0cmFjdFpvb2tlZXBlckNsaWVudC5qYXZh) | `68.67% <42.85%> (-8.75%)` | `0 <0> (ø)` | |
| [.../configcenter/support/zookeeper/CacheListener.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tY29uZmlnY2VudGVyL2R1YmJvLWNvbmZpZ2NlbnRlci16b29rZWVwZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL2NvbmZpZ2NlbnRlci9zdXBwb3J0L3pvb2tlZXBlci9DYWNoZUxpc3RlbmVyLmphdmE=) | `57.14% <43.75%> (-8.72%)` | `0 <0> (ø)` | |
| [...org/apache/dubbo/remoting/zookeeper/EventType.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvRXZlbnRUeXBlLmphdmE=) | `52.94% <52.94%> (ø)` | `0 <0> (?)` | |
| [...ting/zookeeper/curator/CuratorZookeeperClient.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3Rpbmctem9va2VlcGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy96b29rZWVwZXIvY3VyYXRvci9DdXJhdG9yWm9va2VlcGVyQ2xpZW50LmphdmE=) | `67.88% <75%> (+0.17%)` | `0 <0> (ø)` | :arrow_down: |
| [...ache/dubbo/remoting/transport/AbstractChannel.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvQWJzdHJhY3RDaGFubmVsLmphdmE=) | `75% <0%> (-12.5%)` | `0% <0%> (ø)` | |
| [.../remoting/transport/netty4/NettyServerHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctbmV0dHk0L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvbmV0dHk0L05ldHR5U2VydmVySGFuZGxlci5qYXZh) | `61.9% <0%> (-9.53%)` | `0% <0%> (ø)` | |
| [...ng/transport/dispatcher/all/AllChannelHandler.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy90cmFuc3BvcnQvZGlzcGF0Y2hlci9hbGwvQWxsQ2hhbm5lbEhhbmRsZXIuamF2YQ==) | `51.42% <0%> (-5.72%)` | `0% <0%> (ø)` | |
| [.../org/apache/dubbo/remoting/ExecutionException.java](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree#diff-ZHViYm8tcmVtb3RpbmcvZHViYm8tcmVtb3RpbmctYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZW1vdGluZy9FeGVjdXRpb25FeGNlcHRpb24uamF2YQ==) | `15.78% <0%> (-5.27%)` | `0% <0%> (ø)` | |
| ... and [11 more](https://codecov.io/gh/apache/incubator-dubbo/pull/3603/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=footer). Last update [c65b589...101d09e](https://codecov.io/gh/apache/incubator-dubbo/pull/3603?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] chickenlj commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "chickenlj (GitHub)" <gi...@apache.org>.

Do we need `removeDataListener`?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] cvictory commented on pull request #3603: fix #3288. make configcenter use zookeeperClient defined by dubbo

Posted by "cvictory (GitHub)" <gi...@apache.org>.

It creates one instance in ZookeeperDynamicConfiguration. 
You can check it again.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/3603 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org