fix #750. registry center notify with complete address list #758

cvictory · 2020-09-16T07:12:54Z

1.fix empty ServiceEvent, we should clear all invokers
2.we want to notify the address with complete address list.

…s 2.we want to notify the address with complete address list.

AlexStocks · 2020-09-16T11:32:38Z

registry/nacos/registry_test.go

@@ -35,6 +36,9 @@ import (
 )

 func TestNacosRegistry_Register(t *testing.T) {
+	if _, err := http.Get("http://console.nacos.io/nacos/"); err != nil {


maybe u'd better encapsulate a func to delete duplicate codes.

beiwei30 · 2020-09-16T14:51:04Z

registry/directory/directory.go

 	for _, event := range events {
+		// Is the key (url.Key()) of cacheInvokersMap the best way?


this is necessary, we should assume (safely) that all events are UPDATE events, otherwise how would you process DELETE events? plus, per the comment on the interface:

// The argument of events []*ServiceEvent is equal to urls []*URL, because Action of ServiceEvent will be ignored.

…otifyAll func

…oker list from cache

zouyx · 2020-10-01T15:35:25Z

registry/protocol/protocol.go

+	if len(events) == 0 {
+		return
+	}
+
+	event := events[0]
+	nl.Notify(event)
+}


why notify all just get first element？

zouyx · 2020-10-01T15:40:20Z

cluster/router/chain/chain.go

-	if invokers == nil || len(invokers) == 0 {
-		return
-	}


why delete this block？

delete empty protection. When the address is empty, the route should be refresh.

hxmhlt

LGTM

zouyx

Merge develop and make Checks to normal.

AlexStocks · 2020-10-16T13:39:36Z

@hequn @lizhixin

codecov-io · 2020-11-13T03:41:36Z

Codecov Report

Merging #758 (0cb6305) into develop (ab774b5) will decrease coverage by 0.03%.
The diff coverage is 69.44%.

@@             Coverage Diff             @@
##           develop     #758      +/-   ##
===========================================
- Coverage    60.08%   60.04%   -0.04%     
===========================================
  Files          260      260              
  Lines        12856    12896      +40     
===========================================
+ Hits          7725     7744      +19     
- Misses        4167     4185      +18     
- Partials       964      967       +3

Impacted Files	Coverage Δ
cluster/router/chain/chain.go	`68.54% <ø> (-2.09%)`	⬇️
registry/nacos/service_discovery.go	`64.13% <0.00%> (-0.45%)`	⬇️
registry/protocol/protocol.go	`76.96% <0.00%> (-0.88%)`	⬇️
registry/directory/directory.go	`79.72% <75.75%> (+5.71%)`	⬆️
config_center/nacos/facade.go	`79.31% <0.00%> (-10.35%)`	⬇️
metadata/report/delegate/delegate_report.go	`29.66% <0.00%> (-9.33%)`	⬇️
config_center/nacos/client.go	`65.26% <0.00%> (-2.11%)`	⬇️
remoting/getty/pool.go	`68.72% <0.00%> (-0.89%)`	⬇️

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 ab774b5...0cb6305. Read the comment docs.

LaurenceLiZhixin · 2020-12-10T09:56:48Z

registry/directory/directory.go

+// refreshInvokers refreshes service's events.
+func (dir *RegistryDirectory) refreshInvokers(event *registry.ServiceEvent) {
+	if event != nil {
+		logger.Debugf("refresh invokers with %+v")


Event detail may be left in func Debugf

LaurenceLiZhixin · 2020-12-10T10:01:13Z

LGTM

This reverts commit af608d8.

…_all

fix #750. registry center notify with complete address list

fix apache#750. 1.fix empty ServiceEvent, we should clear all invoker…

3053a22

…s 2.we want to notify the address with complete address list.

cvictory added this to the 1.5.2 milestone Sep 16, 2020

cvictory added 2 commits September 16, 2020 15:21

add test for clear all address

fa94763

fix nacos test

c3aac14

AlexStocks reviewed Sep 16, 2020

View reviewed changes

AlexStocks approved these changes Sep 16, 2020

View reviewed changes

fix nacos test

2a6c0a1

beiwei30 reviewed Sep 16, 2020

View reviewed changes

cvictory added 3 commits September 23, 2020 20:41

fix review issue: add constraint of Action value of ServiceEvent in N…

94496c5

…otifyAll func

Merge branch 'develop' of github.com:apache/dubbo-go into notify_all

eb1b730

fix apache#750. 1.fix unit test

6053327

cvictory force-pushed the notify_all branch from 3352d96 to 6053327 Compare September 25, 2020 03:06

cvictory requested a review from beiwei30 September 25, 2020 09:45

cvictory added 2 commits September 27, 2020 14:42

Merge branch 'develop' of github.com:apache/dubbo-go into notify_all

b90bb44

fix: if the registry center notify address list, it cannot remove inv…

3389b38

…oker list from cache

zouyx reviewed Oct 1, 2020

View reviewed changes

hxmhlt approved these changes Oct 6, 2020

View reviewed changes

zouyx reviewed Oct 10, 2020

View reviewed changes

cvictory added 4 commits October 13, 2020 21:29

enhance the refresh func

634e8d2

remove doc

45c926b

enhance the refresh func

5f31e3a

Merge branch 'develop' of github.com:apache/dubbo-go into notify_all

0c94049

zouyx modified the milestones: 1.5.2, v1.5.3 Oct 16, 2020

add some debug log

d5ffb62

cvictory force-pushed the develop branch from 04bb28d to ea9d6eb Compare October 27, 2020 05:15

cvictory added 2 commits November 13, 2020 11:38

use goroutine to destroy invoker

2757a1a

Merge branch 'develop' of github.com:apache/dubbo-go into notify_all

51ee140

cvictory added 3 commits December 9, 2020 15:24

fix merge issue and optimize some invalid usage

60f53ab

fix log error

f4d5df2

refactor some code

068505d

LaurenceLiZhixin reviewed Dec 10, 2020

View reviewed changes

zouyx added 2 commits December 10, 2020 23:15

trigger init router for first call

af608d8

Revert "trigger init router for first call"

a5bd416

This reverts commit af608d8.

zouyx removed this from the v1.5.5 milestone Dec 11, 2020

cvictory added 3 commits December 14, 2020 08:28

fix review issue

5ffcdfe

Merge branch 'develop' of github.com:apache/dubbo-go into notify_all

a136af5

Merge branch 'notify_all' of github.com:cvictory/dubbo-go into notify…

0cb6305

…_all

zouyx modified the milestones: v1.5.5, v1.5.6 Dec 18, 2020

zouyx approved these changes Dec 18, 2020

View reviewed changes

zouyx merged commit cfef02a into apache:develop Dec 18, 2020

AlexStocks pushed a commit that referenced this pull request Apr 14, 2021

Merge pull request #758 from cvictory/notify_all

fdbe273

fix #750. registry center notify with complete address list

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #750. registry center notify with complete address list #758

fix #750. registry center notify with complete address list #758

cvictory commented Sep 16, 2020

AlexStocks Sep 16, 2020

cvictory Sep 18, 2020

beiwei30 Sep 16, 2020

zouyx Oct 1, 2020

cvictory Dec 9, 2020

zouyx Oct 1, 2020

cvictory Dec 14, 2020

hxmhlt left a comment

zouyx left a comment

AlexStocks commented Oct 16, 2020

codecov-io commented Nov 13, 2020 •

edited

Loading

LaurenceLiZhixin Dec 10, 2020

cvictory Dec 14, 2020

LaurenceLiZhixin commented Dec 10, 2020

		for _, event := range events {
		// Is the key (url.Key()) of cacheInvokersMap the best way?

fix #750. registry center notify with complete address list #758

fix #750. registry center notify with complete address list #758

Conversation

cvictory commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hxmhlt left a comment

Choose a reason for hiding this comment

zouyx left a comment

Choose a reason for hiding this comment

AlexStocks commented Oct 16, 2020

codecov-io commented Nov 13, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurenceLiZhixin commented Dec 10, 2020

codecov-io commented Nov 13, 2020 •

edited

Loading