-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Use nodeCache to take place of treeCache. #7466
Conversation
Codecov Report
@@ 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 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -38,15 +37,12 @@ | |||
*/ | |||
|
|||
public class CacheListener implements DataListener { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ...)
} | ||
|
||
@Override | ||
protected void doRemoveListener(String pathKey, ConfigurationListener listener) { | ||
cacheListener.removeListener(pathKey, listener); | ||
zkClient.removeDataListener(pathKey, cacheListener); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
LGTM |
java.lang.NoSuchMethodError: org.apache.curator.framework.recipes.cache.NodeCache.getListenable |
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; |
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 |
I did an analysis, please see the link below. |
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. |
What is the purpose of the change
use nodeCache to take place of treeCache.