Ftr : add UnRegister and UnSubscribe for zookeeper Registry #510

zouyx · 2020-05-10T14:50:42Z

What this PR does:

add UnRegister and UnSubscribe for Registry

Which issue(s) this PR fixes:

Fixes #489

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

codecov-io · 2020-05-11T07:50:52Z

Codecov Report

Merging #510 into feature/dubbo-2.7.5 will increase coverage by 0.19%.
The diff coverage is 69.54%.

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #510      +/-   ##
=======================================================
+ Coverage                66.38%   66.57%   +0.19%     
=======================================================
  Files                      184      199      +15     
  Lines                     9717    10275     +558     
=======================================================
+ Hits                      6451     6841     +390     
- Misses                    2626     2776     +150     
- Partials                   640      658      +18

Impacted Files	Coverage Δ
common/extension/event_dispatcher.go	`0.00% <0.00%> (ø)`
common/extension/metadata_report_factory.go	`0.00% <0.00%> (ø)`
...ommon/observer/dispatcher/mock_event_dispatcher.go	`0.00% <0.00%> (ø)`
common/rpc_service.go	`85.18% <0.00%> (-1.07%)`	⬇️
config/base_config.go	`64.14% <ø> (ø)`
registry/etcdv3/registry.go	`69.01% <0.00%> (-4.13%)`	⬇️
registry/kubernetes/registry.go	`67.88% <0.00%> (-3.27%)`	⬇️
registry/nacos/registry.go	`55.31% <0.00%> (-3.11%)`	⬇️
remoting/zookeeper/listener.go	`51.09% <0.00%> (-0.29%)`	⬇️
common/observer/event.go	`16.66% <16.66%> (ø)`
... 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 24e7205...ab046ef. Read the comment docs.

filter registry url params starting with dot

registry/zookeeper/registry.go

registry/base_registry.go

registry/etcdv3/registry.go

registry/zookeeper/listener.go

registry/zookeeper/registry.go

registry/base_registry.go

registry/etcdv3/registry.go

registry/nacos/registry.go

registry/base_registry.go

registry/etcdv3/registry.go

registry/base_registry.go

remoting/zookeeper/listener.go

registry/base_registry.go

pantianying · 2020-05-16T11:46:45Z

suggested to run off the scene of ZK disconnection at last

hxmhlt

LGTM

registry/mock_registry.go

fangyincheng · 2020-05-17T18:16:01Z

registry/zookeeper/registry_test.go

+	err := reg.Register(url)
+	assert.NoError(t, err)
+
+	if err != nil {


why add err != nil? by assert.NoError(t, err), when err != nil, the ut will fail.

This block is the same to Test_subsribe ,or you mean Test_subscribe is incorrect too ?

dubbo-go/registry/zookeeper/registry_test.go

Line 57 in 8a3c02c

if err != nil {

it is not necessary. i think it is better to delete them. or other resons?

@hxmhlt can you remember the reason? If not , can i delete it?

cch123 · 2020-05-18T05:46:57Z

LGTM

AlexStocks · 2020-05-18T06:25:09Z

LGTM

have u tested this pr?

registry/base_registry.go

pantianying

LGTM

registry/kubernetes/registry.go

registry/etcdv3/registry.go

registry/zookeeper/registry.go

codecov-commenter · 2020-05-19T00:37:50Z

Codecov Report

Merging #510 into feature/dubbo-2.7.5 will increase coverage by 0.37%.
The diff coverage is 69.78%.

@@                   Coverage Diff                   @@
##           feature/dubbo-2.7.5     #510      +/-   ##
=======================================================
+ Coverage                66.38%   66.76%   +0.37%     
=======================================================
  Files                      184      199      +15     
  Lines                     9717    10272     +555     
=======================================================
+ Hits                      6451     6858     +407     
- Misses                    2626     2755     +129     
- Partials                   640      659      +19

Impacted Files	Coverage Δ
common/extension/event_dispatcher.go	`0.00% <0.00%> (ø)`
common/extension/metadata_report_factory.go	`0.00% <0.00%> (ø)`
...ommon/observer/dispatcher/mock_event_dispatcher.go	`0.00% <0.00%> (ø)`
common/rpc_service.go	`85.18% <0.00%> (-1.07%)`	⬇️
config/base_config.go	`64.14% <ø> (ø)`
registry/etcdv3/registry.go	`69.01% <0.00%> (-4.13%)`	⬇️
registry/kubernetes/registry.go	`68.51% <0.00%> (-2.64%)`	⬇️
registry/nacos/registry.go	`55.31% <0.00%> (-3.11%)`	⬇️
remoting/zookeeper/listener.go	`51.09% <0.00%> (-0.29%)`	⬇️
common/observer/event.go	`16.66% <16.66%> (ø)`
... and 39 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 24e7205...7e7db7b. Read the comment docs.

registry/base_registry.go

AlexStocks · 2020-05-19T07:40:35Z

registry/base_registry.go

+func (r *BaseRegistry) createPath(dubboPath string) error {
+	r.cltLock.Lock()
+	defer r.cltLock.Unlock()
+	return r.facadeBasedRegistry.CreatePath(dubboPath)


have u got the reason of lock facadeBasedRegistry.CreatePath by r.cltLock.Lock()?

the source code as below, i just extract it.

dubbo-go/registry/base_registry.go

Line 240 in 8a3c02c

r.cltLock.Lock()

dubbo-go/registry/base_registry.go

Line 288 in 8a3c02c

r.cltLock.Lock()

dubbo-go/registry/base_registry.go

Line 300 in 8a3c02c

r.cltLock.Lock()

registry/base_registry.go

zouyx and others added 5 commits May 4, 2020 23:08

add interface for unregister and unsubscribe

62b3787

filter registry url params starting with dot

d1e6d85

add unit test

be8dd32

add unregister for Registry and FacadeBasedRegistry

3e81841

add test case for Registry unregister

df74f8d

zouyx changed the title ~~[WIP] Ftr : add UnRegister and UnSubscribe for Registry~~ [WIP] Ftr : add UnRegister and UnSubscribe for zookeeper Registry May 10, 2020

AlexStocks requested review from hxmhlt and flycash and removed request for hxmhlt May 11, 2020 03:22

zouyx marked this pull request as draft May 11, 2020 06:56

zouyx added 2 commits May 11, 2020 15:16

modify test case for Registry unregister

4970301

modify test case for Registry unregister

69ace5e

zouyx and others added 4 commits May 11, 2020 23:18

add Registry unsubscribe

322eb5f

Merge pull request apache#509 from CodingSinger/HIDE_KEY

d90c20e

filter registry url params starting with dot

change lock granularity

31f8663

add panic for not support implement

341bc5f

CodingSinger reviewed May 13, 2020

View reviewed changes

registry/zookeeper/registry.go Show resolved Hide resolved

hxmhlt reviewed May 13, 2020

View reviewed changes

pantianying reviewed May 13, 2020

View reviewed changes

registry/base_registry.go Outdated Show resolved Hide resolved

zouyx marked this pull request as ready for review May 13, 2020 02:25

flycash reviewed May 13, 2020

View reviewed changes

registry/base_registry.go Outdated Show resolved Hide resolved

registry/base_registry.go Outdated Show resolved Hide resolved

flycash reviewed May 13, 2020

View reviewed changes

registry/etcdv3/registry.go Outdated Show resolved Hide resolved

registry/nacos/registry.go Outdated Show resolved Hide resolved

registry/nacos/registry.go Outdated Show resolved Hide resolved

fix review problems

f1d5aa4

fangyincheng reviewed May 13, 2020

View reviewed changes

registry/base_registry.go Show resolved Hide resolved

registry/etcdv3/registry.go Outdated Show resolved Hide resolved

zouyx added 2 commits May 15, 2020 16:21

Merge branch 'develop' into feature/addRegistryUnpub

1b55d1e

fix review comments

ce89e9b

AlexStocks reviewed May 16, 2020

View reviewed changes

registry/base_registry.go Show resolved Hide resolved

remoting/zookeeper/listener.go Outdated Show resolved Hide resolved

fix review comments

50c24b1

zouyx changed the title ~~[WIP] Ftr : add UnRegister and UnSubscribe for zookeeper Registry~~ Ftr : add UnRegister and UnSubscribe for zookeeper Registry May 16, 2020

add comment and test case

f3a7845

zouyx assigned zouyx, hxmhlt and pantianying and unassigned zouyx May 16, 2020

pantianying reviewed May 16, 2020

View reviewed changes

registry/base_registry.go Show resolved Hide resolved

hxmhlt approved these changes May 17, 2020

View reviewed changes

fangyincheng reviewed May 17, 2020

View reviewed changes

zouyx added 2 commits May 18, 2020 18:19

fix review comments

7a87b5a

fix review comments

ab046ef

AlexStocks reviewed May 18, 2020

View reviewed changes

registry/base_registry.go Outdated Show resolved Hide resolved

pantianying approved these changes May 18, 2020

View reviewed changes

flycash reviewed May 18, 2020

View reviewed changes

registry/kubernetes/registry.go Outdated Show resolved Hide resolved

registry/etcdv3/registry.go Outdated Show resolved Hide resolved

registry/zookeeper/registry.go Outdated Show resolved Hide resolved

fix review problems

2699fcc

fix review problems

f0c9c8f

flycash approved these changes May 19, 2020

View reviewed changes

AlexStocks reviewed May 19, 2020

View reviewed changes

fix review problems

7e7db7b

AlexStocks merged commit 9026120 into apache:feature/dubbo-2.7.5 May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ftr : add UnRegister and UnSubscribe for zookeeper Registry #510

Ftr : add UnRegister and UnSubscribe for zookeeper Registry #510

zouyx commented May 10, 2020

codecov-io commented May 11, 2020 •

edited

Loading

pantianying commented May 16, 2020

hxmhlt left a comment

fangyincheng May 17, 2020

zouyx May 18, 2020

fangyincheng May 18, 2020

zouyx May 19, 2020

cch123 commented May 18, 2020

AlexStocks commented May 18, 2020

pantianying left a comment

codecov-commenter commented May 19, 2020 •

edited

Loading

AlexStocks May 19, 2020

zouyx May 19, 2020

Ftr : add UnRegister and UnSubscribe for zookeeper Registry #510

Ftr : add UnRegister and UnSubscribe for zookeeper Registry #510

Conversation

zouyx commented May 10, 2020

codecov-io commented May 11, 2020 • edited Loading

Codecov Report

pantianying commented May 16, 2020

hxmhlt left a comment

Choose a reason for hiding this comment

fangyincheng May 17, 2020

Choose a reason for hiding this comment

zouyx May 18, 2020

Choose a reason for hiding this comment

fangyincheng May 18, 2020

Choose a reason for hiding this comment

zouyx May 19, 2020

Choose a reason for hiding this comment

cch123 commented May 18, 2020

AlexStocks commented May 18, 2020

pantianying left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 19, 2020 • edited Loading

Codecov Report

AlexStocks May 19, 2020

Choose a reason for hiding this comment

zouyx May 19, 2020

Choose a reason for hiding this comment

codecov-io commented May 11, 2020 •

edited

Loading

codecov-commenter commented May 19, 2020 •

edited

Loading