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.

Sign up for GitHub

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

Jump to bottom

Use nodeCache to take place of treeCache. #7466

Merged
merged 10 commits into from
Apr 6, 2021
Merged

Use nodeCache to take place of treeCache. #7466

merged 10 commits into from
Apr 6, 2021

Conversation

horizonzy
Copy link
Member

What is the purpose of the change

use nodeCache to take place of treeCache.

@codecov-io
Copy link

codecov-io commented Mar 29, 2021 •
edited

Codecov Report

Merging #7466 (0be79b2) into master (041f8bb) will decrease coverage by 0.14%.
The diff coverage is 65.45%.

❗ Current head 0be79b2 differs from pull request most recent head bc2a24b. Consider uploading reports for the commit bc2a24b to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7466      +/-   ##
============================================
- Coverage     59.37%   59.22%   -0.15%     
- Complexity      501      503       +2     
============================================
  Files          1077     1079       +2     
  Lines         43306    43378      +72     
  Branches       6314     6329      +15     
============================================
- Hits          25713    25692      -21     
- Misses        14752    14852     +100     
+ Partials       2841     2834       -7
Impacted Files Coverage Δ Complexity Δ
...pport/zookeeper/ZookeeperDynamicConfiguration.java 55.55% <33.33%> (-6.52%) 0.00 <0.00> (ø)
.../configcenter/support/zookeeper/CacheListener.java 56.25% <58.33%> (-3.75%) 0.00 <0.00> (ø)
...ing/zookeeper/support/AbstractZookeeperClient.java 74.68% <66.66%> (ø) 0.00 <0.00> (ø)
...ting/zookeeper/curator/CuratorZookeeperClient.java 67.81% <77.41%> (+3.26%) 0.00 <0.00> (ø)
...gcenter/wrapper/CompositeDynamicConfiguration.java 0.00% <0.00%> (-50.00%) 0.00% <0.00%> (ø%)
...gistry/client/migration/MigrationRuleListener.java 55.88% <0.00%> (-17.65%) 0.00% <0.00%> (ø%)
...o/rpc/cluster/support/migration/MigrationRule.java 46.42% <0.00%> (-12.20%) 0.00% <0.00%> (ø%)
...t/migration/DefaultMigrationAddressComparator.java 15.38% <0.00%> (-11.54%) 0.00% <0.00%> (ø%)
...xt/DubboLifecycleComponentApplicationListener.java 92.30% <0.00%> (-7.70%) 0.00% <0.00%> (ø%)
...bo/registry/client/migration/MigrationInvoker.java 28.57% <0.00%> (-5.36%) 0.00% <0.00%> (ø%)
... and 38 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 041f8bb...bc2a24b. Read the comment docs.

AlbumenJ
AlbumenJ approved these changes Mar 29, 2021
View reviewed changes
Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

LGTM.

chickenlj
chickenlj requested changes Apr 2, 2021
View reviewed changes
...r-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/CacheListener.java
@@ -38,15 +37,12 @@
*/

public class CacheListener implements DataListener {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is CacheListener still needed? Can we connect TargetListener(zookeeper listener) and DataListener(dubbo listener) directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

CacheListener can hold same path listeners, ZookeeperListener : ConfigurationListener -> 1 : N.
We still use CacheListener, ZookeeperListener : CacheListener -> 1 : 1, CacheListener : ConfigurationListener -> 1 : N.

CacheListener is an adapter, it holds real listeners, and adapt kinds of listener (zookeeper, nacos ...)

...main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java Outdated
}

@Override
protected void doRemoveListener(String pathKey, ConfigurationListener listener) {
cacheListener.removeListener(pathKey, listener);
zkClient.removeDataListener(pathKey, cacheListener);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if cacheListener holds more than one DataListener? Should we check dataListener size before stop listening from zookeeper

Copy link
Member Author

Choose a reason for hiding this comment

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

right.

...keeper/src/main/java/org/apache/dubbo/remoting/zookeeper/curator/CuratorZookeeperClient.java Outdated Show resolved Hide resolved
@chickenlj
Copy link
Contributor

LGTM

@chickenlj chickenlj merged commit 2de7c4a into apache:master Apr 6, 2021
@NickZhang1024
Copy link

java.lang.NoSuchMethodError: org.apache.curator.framework.recipes.cache.NodeCache.getListenable

@NickZhang1024
Copy link

duboo 2.7.10 is ok, update to 2.7.11, error : java.lang.NoSuchMethodError: org.apache.curator.framework.recipes.cache.NodeCache.getListenable()Lorg/apache/curator/framework/listen/ListenerContainer;
java.lang.IllegalStateException: Extension instance (name: registry, class: interface org.apache.dubbo.rpc.Protocol) couldn't be instantiated: null

@horizonzy
Copy link
Member Author

horizonzy commented May 17, 2021 •
edited

duboo 2.7.10 is ok, update to 2.7.11, error : java.lang.NoSuchMethodError: org.apache.curator.framework.recipes.cache.NodeCache.getListenable()Lorg/apache/curator/framework/listen/ListenerContainer;
java.lang.IllegalStateException: Extension instance (name: registry, class: interface org.apache.dubbo.rpc.Protocol) couldn't be instantiated: null

Hi, it seems the dependencies conflict, have you solve it? If not , please create an issue to detail descrip it. Put the result which from mvn dependency:tree

@horizonzy horizonzy mentioned this pull request Jun 1, 2021
Merged
@harvies
Copy link

harvies commented Jul 8, 2021

java.lang.NoSuchMethodError: org.apache.curator.framework.recipes.cache.NodeCache.getListenable

I did an analysis, please see the link below.
#7772 (comment)

@harvies
Copy link

harvies commented Jul 8, 2021

What problem does this pr solve? The pr brings some new compatibility issues. Please see #7772.

@horizonzy
Copy link
Member Author

What problem does this pr solve? The pr brings some new compatibility issues. Please see #7772.

Hi, we use treeCache to watch all children of / before, If the children's number to much, it may cause out of memory.
So we change the watch way, use nodeCache to watch particular zk node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chickenlj chickenlj chickenlj requested changes

@AlbumenJ AlbumenJ AlbumenJ 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.

None yet
6 participants
@horizonzy @codecov-io @chickenlj @NickZhang1024 @harvies @AlbumenJ