You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2021/08/06 06:23:43 UTC

[GitHub] [dubbo-go] chickenlj opened a new pull request #1368: Refactor to remove event dispatcher completely

chickenlj opened a new pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368


   The event dispatcher is flexible but not necessary for Dubbo for it makes the code and workflow very hard to follow. Event can be simply replaced with well-defined Listeners.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] Mulavar commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
Mulavar commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r689994596



##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -282,7 +287,15 @@ func (e *etcdV3ServiceDiscovery) DataChange(eventType remoting.Event) bool {
 			instance.ServiceName = ""
 		}
 
-		if err := e.DispatchEventByServiceName(instance.ServiceName); err != nil {
+		// notify instance listener instance change
+		name := instance.ServiceName // TODO, seems like ServiceName is not exactly the app name?

Review comment:
       pls deal with this ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695119076



##########
File path: registry/zookeeper/service_discovery.go
##########
@@ -306,7 +310,15 @@ func (zksd *zookeeperServiceDiscovery) DataChange(eventType remoting.Event) bool
 	path = strings.TrimPrefix(path, constant.PATH_SEPARATOR)
 	// get service name in zk path
 	serviceName := strings.Split(path, constant.PATH_SEPARATOR)[0]
-	err := zksd.DispatchEventByServiceName(serviceName)
+
+	var err error
+	instances := zksd.GetInstances(serviceName)
+	for _, lis := range zksd.instanceListenerMap[serviceName].Values() {
+		var instanceListener registry.ServiceInstancesChangedListener
+		instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       这里不对断言结果检查的话应该会导致 panic




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
AlexStocks commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r715325770



##########
File path: registry/servicediscovery/service_discovery_registry.go
##########
@@ -352,5 +344,26 @@ func tryInitMetadataService(url *common.URL) {
 	if err != nil {
 		logger.Errorf("could not export the metadata service", err)
 	}
-	extension.GetGlobalDispatcher().Dispatch(event.NewServiceConfigExportedEvent(expt.(*configurable.MetadataServiceExporter).ServiceConfig))
+
+	// report interface-app mapping
+	err = publishMapping(expt.(*configurable.MetadataServiceExporter).ServiceConfig)
+	if err != nil {
+		logger.Errorf("Publish interface-application mapping failed", err)

Review comment:
       I have fixed it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] chickenlj commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
chickenlj commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r704094916



##########
File path: registry/zookeeper/service_discovery.go
##########
@@ -306,7 +310,15 @@ func (zksd *zookeeperServiceDiscovery) DataChange(eventType remoting.Event) bool
 	path = strings.TrimPrefix(path, constant.PATH_SEPARATOR)
 	// get service name in zk path
 	serviceName := strings.Split(path, constant.PATH_SEPARATOR)[0]
-	err := zksd.DispatchEventByServiceName(serviceName)
+
+	var err error
+	instances := zksd.GetInstances(serviceName)
+	for _, lis := range zksd.instanceListenerMap[serviceName].Values() {
+		var instanceListener registry.ServiceInstancesChangedListener
+		instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       我认为panic是预期内的，这是运行态非预期内行为，就是要以失败结束，再额外捕获我们做不了什么事情。
   
   这里之所有需要转换，是因为 gxset.HashSet 是个通用的结构，存储了 Interface{}

##########
File path: registry/nacos/service_discovery.go
##########
@@ -240,7 +260,13 @@ func (n *nacosServiceDiscovery) AddListener(listener registry.ServiceInstancesCh
 					})
 				}
 
-				e := n.DispatchEventForInstances(serviceName, instances)
+				var e error
+				for _, lis := range n.instanceListenerMap[serviceName].Values() {
+					var instanceListener registry.ServiceInstancesChangedListener
+					instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       我认为panic是预期内的，这是运行态非预期内行为，就是要以失败结束，再额外捕获我们做不了什么事情。
   
   这里之所有需要转换，是因为 gxset.HashSet 是个通用的结构，存储了 Interface{}




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] codecov-commenter commented on pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#issuecomment-894041250


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1368](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f201c97) into [3.0](https://codecov.io/gh/apache/dubbo-go/commit/0376b53e551441805c2f379bd9077aeed2b250b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0376b53) will **decrease** coverage by `0.15%`.
   > The diff coverage is `29.62%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/1368/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##              3.0    #1368      +/-   ##
   ==========================================
   - Coverage   54.80%   54.64%   -0.16%     
   ==========================================
     Files         272      260      -12     
     Lines       14736    14490     -246     
   ==========================================
   - Hits         8076     7918     -158     
   + Misses       5785     5711      -74     
   + Partials      875      861      -14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [config/config\_loader\_options.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnL2NvbmZpZ19sb2FkZXJfb3B0aW9ucy5nbw==) | `66.66% <ø> (ø)` | |
   | [...gistry/event/protocol\_ports\_metadata\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvcHJvdG9jb2xfcG9ydHNfbWV0YWRhdGFfY3VzdG9taXplci5nbw==) | `10.81% <0.00%> (-18.92%)` | :arrow_down: |
   | [registry/event/service\_revision\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvc2VydmljZV9yZXZpc2lvbl9jdXN0b21pemVyLmdv) | `14.89% <0.00%> (-27.66%)` | :arrow_down: |
   | [registry/file/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZmlsZS9zZXJ2aWNlX2Rpc2NvdmVyeS5nbw==) | `50.90% <ø> (+3.04%)` | :arrow_up: |
   | [...try/servicediscovery/service\_discovery\_registry.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvc2VydmljZWRpc2NvdmVyeS9zZXJ2aWNlX2Rpc2NvdmVyeV9yZWdpc3RyeS5nbw==) | `23.71% <5.55%> (-1.02%)` | :arrow_down: |
   | [config/config\_loader.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnL2NvbmZpZ19sb2FkZXIuZ28=) | `49.41% <20.00%> (-0.40%)` | :arrow_down: |
   | [registry/etcdv3/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXRjZHYzL3NlcnZpY2VfZGlzY292ZXJ5Lmdv) | `17.72% <23.33%> (+2.00%)` | :arrow_up: |
   | [registry/nacos/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvbmFjb3Mvc2VydmljZV9kaXNjb3ZlcnkuZ28=) | `67.55% <58.33%> (-2.91%)` | :arrow_down: |
   | [...ry/event/metadata\_service\_url\_params\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvbWV0YWRhdGFfc2VydmljZV91cmxfcGFyYW1zX2N1c3RvbWl6ZXIuZ28=) | `43.75% <100.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0376b53...f201c97](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] flycash commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
flycash commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r704234145



##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -215,31 +216,19 @@ func (e *etcdV3ServiceDiscovery) GetRequestInstances(serviceNames []string, offs
 // see addServiceInstancesChangedListener in Java
 func (e *etcdV3ServiceDiscovery) AddListener(listener registry.ServiceInstancesChangedListener) error {
 	for _, t := range listener.GetServiceNames().Values() {
-		serviceName := t.(string)
-		err := e.registerSreviceWatcher(serviceName)
+		err2 := e.registerServiceInstanceListener(t.(string), listener)
+		if err2 != nil {
+			return err2
+		}
+
+		err := e.registerServiceWatcher(t.(string))

Review comment:
       ```go
   err := e.registerServiceInstanceListener(t.(string), listener)
   // ....
   err = e.registerServiceWatcher(t.(string))
   ```
   looks better?

##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -282,7 +287,15 @@ func (e *etcdV3ServiceDiscovery) DataChange(eventType remoting.Event) bool {
 			instance.ServiceName = ""
 		}
 
-		if err := e.DispatchEventByServiceName(instance.ServiceName); err != nil {
+		// notify instance listener instance change
+		name := instance.ServiceName
+		instances := e.GetInstances(name)
+		for _, lis := range e.instanceListenerMap[instance.ServiceName].Values() {

Review comment:
       I am not sure if we should iterate all listeners even some of them return error.

##########
File path: registry/nacos/service_discovery.go
##########
@@ -210,8 +211,27 @@ func (n *nacosServiceDiscovery) GetRequestInstances(serviceNames []string, offse
 	return res
 }
 
+func (n *nacosServiceDiscovery) registerInstanceListener(listener registry.ServiceInstancesChangedListener) {
+	for _, t := range listener.GetServiceNames().Values() {

Review comment:
       it's not thread safe here. Should we think about `race condition`?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] flycash commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
flycash commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r710072206



##########
File path: registry/servicediscovery/service_discovery_registry.go
##########
@@ -352,5 +344,26 @@ func tryInitMetadataService(url *common.URL) {
 	if err != nil {
 		logger.Errorf("could not export the metadata service", err)
 	}
-	extension.GetGlobalDispatcher().Dispatch(event.NewServiceConfigExportedEvent(expt.(*configurable.MetadataServiceExporter).ServiceConfig))
+
+	// report interface-app mapping
+	err = publishMapping(expt.(*configurable.MetadataServiceExporter).ServiceConfig)
+	if err != nil {
+		logger.Errorf("Publish interface-application mapping failed", err)

Review comment:
       forget placeholader %v ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695114323



##########
File path: registry/event/service_revision_customizer.go
##########
@@ -18,6 +18,7 @@
 package event
 
 import (
+	"dubbo.apache.org/dubbo-go/v3/metadata/service/local"

Review comment:
       这个也是和上面的一样，放在下面的 import 块比较好




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] chickenlj commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
chickenlj commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r704094916



##########
File path: registry/zookeeper/service_discovery.go
##########
@@ -306,7 +310,15 @@ func (zksd *zookeeperServiceDiscovery) DataChange(eventType remoting.Event) bool
 	path = strings.TrimPrefix(path, constant.PATH_SEPARATOR)
 	// get service name in zk path
 	serviceName := strings.Split(path, constant.PATH_SEPARATOR)[0]
-	err := zksd.DispatchEventByServiceName(serviceName)
+
+	var err error
+	instances := zksd.GetInstances(serviceName)
+	for _, lis := range zksd.instanceListenerMap[serviceName].Values() {
+		var instanceListener registry.ServiceInstancesChangedListener
+		instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       我认为这里正是需要panic的，如果转换失败则是unexpected runtime error, a grave error that can not be recovered，就是要以失败结束，再额外捕获我们做不了什么事情。
   
   这里之所有需要转换，是因为 gxset.HashSet 是个通用的结构，存储了 Interface{}




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695115201



##########
File path: registry/nacos/service_discovery_test.go
##########
@@ -153,8 +147,8 @@ func TestNacosServiceDiscovery_CRUD(t *testing.T) {
 	assert.Equal(t, "b", v)
 
 	// test dispatcher event
-	err = serviceDiscovery.DispatchEventByServiceName(serviceName)
-	assert.Nil(t, err)
+	//err = serviceDiscovery.DispatchEventByServiceName(serviceName)
+	//assert.Nil(t, err)

Review comment:
       这两行注释的代码可以直接删掉




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] chickenlj commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
chickenlj commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r705870282



##########
File path: registry/nacos/service_discovery.go
##########
@@ -210,8 +211,27 @@ func (n *nacosServiceDiscovery) GetRequestInstances(serviceNames []string, offse
 	return res
 }
 
+func (n *nacosServiceDiscovery) registerInstanceListener(listener registry.ServiceInstancesChangedListener) {
+	for _, t := range listener.GetServiceNames().Values() {

Review comment:
       It need to be synchronized.

##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -215,31 +216,19 @@ func (e *etcdV3ServiceDiscovery) GetRequestInstances(serviceNames []string, offs
 // see addServiceInstancesChangedListener in Java
 func (e *etcdV3ServiceDiscovery) AddListener(listener registry.ServiceInstancesChangedListener) error {
 	for _, t := range listener.GetServiceNames().Values() {
-		serviceName := t.(string)
-		err := e.registerSreviceWatcher(serviceName)
+		err2 := e.registerServiceInstanceListener(t.(string), listener)
+		if err2 != nil {
+			return err2
+		}
+
+		err := e.registerServiceWatcher(t.(string))

Review comment:
       Sure, it's better.

##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -282,7 +287,15 @@ func (e *etcdV3ServiceDiscovery) DataChange(eventType remoting.Event) bool {
 			instance.ServiceName = ""
 		}
 
-		if err := e.DispatchEventByServiceName(instance.ServiceName); err != nil {
+		// notify instance listener instance change
+		name := instance.ServiceName
+		instances := e.GetInstances(name)
+		for _, lis := range e.instanceListenerMap[instance.ServiceName].Values() {

Review comment:
       All listeners are of the same type need to be guaranteed. Or we need to change the definition of `instanceListenerMap map[string]*gxset.HashSet` in here, to avoid the defect of Golang does not support generic.

##########
File path: registry/etcdv3/service_discovery.go
##########
@@ -282,7 +287,15 @@ func (e *etcdV3ServiceDiscovery) DataChange(eventType remoting.Event) bool {
 			instance.ServiceName = ""
 		}
 
-		if err := e.DispatchEventByServiceName(instance.ServiceName); err != nil {
+		// notify instance listener instance change
+		name := instance.ServiceName
+		instances := e.GetInstances(name)
+		for _, lis := range e.instanceListenerMap[instance.ServiceName].Values() {

Review comment:
       `instanceListenerMap map[string]map[string][*registry.ServiceInstancesChangedListener]`
   
   or if there's generic support
   
   instanceListenerMap map[string]*gxset.HashSet<string, *registry.ServiceInstancesChangedListener>




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695113416



##########
File path: registry/event/metadata_service_url_params_customizer.go
##########
@@ -18,6 +18,7 @@
 package event
 
 import (
+	"dubbo.apache.org/dubbo-go/v3/metadata/service/local"

Review comment:
       这个放在下面的 import 块比较好




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] AlexStocks merged pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
AlexStocks merged pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] codecov-commenter edited a comment on pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#issuecomment-894041250


   # [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1368](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f201c97) into [3.0](https://codecov.io/gh/apache/dubbo-go/commit/0376b53e551441805c2f379bd9077aeed2b250b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0376b53) will **decrease** coverage by `0.15%`.
   > The diff coverage is `29.62%`.
   
   > :exclamation: Current head f201c97 differs from pull request most recent head 2f288bd. Consider uploading reports for the commit 2f288bd to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go/pull/1368/graphs/tree.svg?width=650&height=150&src=pr&token=dcPE6RyFAL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##              3.0    #1368      +/-   ##
   ==========================================
   - Coverage   54.80%   54.64%   -0.16%     
   ==========================================
     Files         272      260      -12     
     Lines       14736    14490     -246     
   ==========================================
   - Hits         8076     7918     -158     
   + Misses       5785     5711      -74     
   + Partials      875      861      -14     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [config/config\_loader\_options.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnL2NvbmZpZ19sb2FkZXJfb3B0aW9ucy5nbw==) | `66.66% <ø> (ø)` | |
   | [...gistry/event/protocol\_ports\_metadata\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvcHJvdG9jb2xfcG9ydHNfbWV0YWRhdGFfY3VzdG9taXplci5nbw==) | `10.81% <0.00%> (-18.92%)` | :arrow_down: |
   | [registry/event/service\_revision\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvc2VydmljZV9yZXZpc2lvbl9jdXN0b21pemVyLmdv) | `14.89% <0.00%> (-27.66%)` | :arrow_down: |
   | [registry/file/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZmlsZS9zZXJ2aWNlX2Rpc2NvdmVyeS5nbw==) | `50.90% <ø> (+3.04%)` | :arrow_up: |
   | [...try/servicediscovery/service\_discovery\_registry.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvc2VydmljZWRpc2NvdmVyeS9zZXJ2aWNlX2Rpc2NvdmVyeV9yZWdpc3RyeS5nbw==) | `23.71% <5.55%> (-1.02%)` | :arrow_down: |
   | [config/config\_loader.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29uZmlnL2NvbmZpZ19sb2FkZXIuZ28=) | `49.41% <20.00%> (-0.40%)` | :arrow_down: |
   | [registry/etcdv3/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXRjZHYzL3NlcnZpY2VfZGlzY292ZXJ5Lmdv) | `17.72% <23.33%> (+2.00%)` | :arrow_up: |
   | [registry/nacos/service\_discovery.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvbmFjb3Mvc2VydmljZV9kaXNjb3ZlcnkuZ28=) | `67.55% <58.33%> (-2.91%)` | :arrow_down: |
   | [...ry/event/metadata\_service\_url\_params\_customizer.go](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cmVnaXN0cnkvZXZlbnQvbWV0YWRhdGFfc2VydmljZV91cmxfcGFyYW1zX2N1c3RvbWl6ZXIuZ28=) | `43.75% <100.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/dubbo-go/pull/1368/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [0376b53...2f288bd](https://codecov.io/gh/apache/dubbo-go/pull/1368?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695113856



##########
File path: registry/event/protocol_ports_metadata_customizer.go
##########
@@ -18,6 +18,7 @@
 package event
 
 import (
+	"dubbo.apache.org/dubbo-go/v3/metadata/service/local"

Review comment:
       和上面的一样，放在下面的 import 块比较好




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] cjphaha commented on a change in pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
cjphaha commented on a change in pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#discussion_r695119597



##########
File path: registry/nacos/service_discovery.go
##########
@@ -240,7 +260,13 @@ func (n *nacosServiceDiscovery) AddListener(listener registry.ServiceInstancesCh
 					})
 				}
 
-				e := n.DispatchEventForInstances(serviceName, instances)
+				var e error
+				for _, lis := range n.instanceListenerMap[serviceName].Values() {
+					var instanceListener registry.ServiceInstancesChangedListener
+					instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       断言的结果最好检查一下

##########
File path: registry/nacos/service_discovery.go
##########
@@ -240,7 +260,13 @@ func (n *nacosServiceDiscovery) AddListener(listener registry.ServiceInstancesCh
 					})
 				}
 
-				e := n.DispatchEventForInstances(serviceName, instances)
+				var e error
+				for _, lis := range n.instanceListenerMap[serviceName].Values() {
+					var instanceListener registry.ServiceInstancesChangedListener
+					instanceListener = lis.(registry.ServiceInstancesChangedListener)

Review comment:
       个人认为断言的结果最好检查一下




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org

[GitHub] [dubbo-go] chickenlj commented on pull request #1368: Refactor to remove event dispatcher completely

Posted by GitBox <gi...@apache.org>.
chickenlj commented on pull request #1368:
URL: https://github.com/apache/dubbo-go/pull/1368#issuecomment-901559449


   This PR is ready to merge, please take a look. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org