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

Fixes #478. #2150

Closed
wants to merge 1 commit into from
Closed

Fixes #478. #2150

wants to merge 1 commit into from

Conversation

solid210
Copy link

@solid210 solid210 commented Jul 29, 2018

  1. Delete zk node when invoke ReferenceConfig.destory() which consumer uses ZookeeperRegistry.
  2. Remove ConcurrentMap<NotifyListener, ChildListener> in zkListeners if the map is empty(release the memory).

What is the purpose of the change

[Issues-487] Fix 487.

Brief changelog

  1. Delete zk node when invoke ReferenceConfig.destory() which consumer uses ZookeeperRegistry.
  2. Remove ConcurrentMap<NotifyListener, ChildListener> in zkListeners if the map is empty(release the memory).

Verifying this change

1. How to verify zookeeper node?
Prepare:

  • Startup a zookeeper application.
  • Create an simple dubbo provider in an application, use zookeeper registry.
  • Create an simple dubbo consumer in another application, use zookeeper registry.
  • Startup dubbo-admin to monitor provider and consumer node.

Before using this commit:

  1. Create a ReferenceConfig with GenericService in consumer application and use the GenericService to invoke any method of the provider.
  2. Use any browser to open dubbo-admin page and switch to "consumer" tab, you can see there is a consumer data.
  3. Invoke ReferenceConfig#destroy() method, then flush page of dubbo-admin.
  4. You can see the consumer data still exist (because zk node still exist).

After using this commit:
Do same operations 1, 2 and 3 above.
4. You can see the consumer data is removed (zk node is removed.).

2. How to verify zkListener?
There is a JUnit Test to verify zkListener

1. Delete zk node when invoke ReferenceConfig.destory() which consumer uses ZookeeperRegistry.
2. Remove ConcurrentMap<NotifyListener, ChildListener> in zkListeners if the map is empty(release the memory).
@codecov-io
Copy link

Codecov Report

Merging #2150 into master will decrease coverage by 0.08%.
The diff coverage is 36.36%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2150      +/-   ##
============================================
- Coverage     54.23%   54.15%   -0.09%     
+ Complexity     5097     5095       -2     
============================================
  Files           559      559              
  Lines         24945    24955      +10     
  Branches       4445     4448       +3     
============================================
- Hits          13530    13515      -15     
- Misses         9377     9398      +21     
- Partials       2038     2042       +4
Impacted Files Coverage Δ Complexity Δ
...he/dubbo/registry/zookeeper/ZookeeperRegistry.java 63.69% <36.36%> (-2.3%) 32 <1> (+1)
...he/dubbo/remoting/transport/netty/NettyClient.java 72.88% <0%> (-10.17%) 12% <0%> (-1%)
...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java 83.33% <0%> (-5.56%) 6% <0%> (-1%)
.../dubbo/remoting/transport/netty4/NettyChannel.java 61.25% <0%> (-5%) 22% <0%> (-1%)
...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java 41.66% <0%> (-4.17%) 3% <0%> (ø)
...he/dubbo/remoting/transport/netty/NettyServer.java 67.85% <0%> (-3.58%) 8% <0%> (-1%)
...e/dubbo/remoting/transport/netty4/NettyServer.java 72.13% <0%> (-3.28%) 9% <0%> (-1%)
...bo/remoting/transport/netty/NettyCodecAdapter.java 53.12% <0%> (-1.57%) 3% <0%> (ø)
...dubbo/rpc/protocol/dubbo/CallbackServiceCodec.java 77.2% <0%> (-1.48%) 29% <0%> (ø)
...rpc/protocol/dubbo/telnet/InvokeTelnetHandler.java 55.69% <0%> (+3.79%) 13% <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 e90d95c...2efa390. Read the comment docs.

@beiwei30
Copy link
Member

#1792 should have fixed this problem, pls. verify with the last code base on branch 2.7

@beiwei30 beiwei30 closed this Aug 21, 2018
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.

3 participants