[GitHub] [dubbo-go] flycash commented on a change in pull request #510: Ftr : add UnRegister and UnSubscribe for zookeeper Registry

[GitBox <gi...@apache.org>]

flycash commented on a change in pull request #510:
URL: https://github.com/apache/dubbo-go/pull/510#discussion_r426685494



##########
File path: registry/etcdv3/registry.go
##########
@@ -114,6 +114,10 @@ func (r *etcdV3Registry) DoRegister(root string, node string) error {
 	return r.client.Create(path.Join(root, node), "")
 }
 
+func (r *etcdV3Registry) DoUnregister(root string, node string) error {
+	return r.client.Delete(path.Join(root, node))

Review comment:
       Do you need to check the client whether it is valid? Just like the zk implementation?

##########
File path: registry/zookeeper/registry.go
##########
@@ -255,3 +269,39 @@ func (r *zkRegistry) getListener(conf *common.URL) (*RegistryConfigurationListen
 
 	return zkListener, nil
 }
+
+func (r *zkRegistry) getCloseListener(conf *common.URL) (*RegistryConfigurationListener, error) {
+
+	var zkListener *RegistryConfigurationListener
+	r.dataListener.mutex.Lock()
+	configurationListener := r.dataListener.subscribed[conf]
+	if configurationListener != nil {
+
+		zkListener, _ := configurationListener.(*RegistryConfigurationListener)
+		if zkListener != nil {
+			r.listenerLock.Lock()

Review comment:
       It looks like there is not necessary to add lock here because you do not change any system status inside the if-block. It's not the check-do something scenario. 

##########
File path: registry/kubernetes/registry.go
##########
@@ -107,6 +107,11 @@ func (r *kubernetesRegistry) DoRegister(root string, node string) error {
 	return r.client.Create(path.Join(root, node), "")
 }
 
+func (r *kubernetesRegistry) DoUnregister(root string, node string) error {
+	panic("DoUnregister is not support in kubernetesRegistry")

Review comment:
       Why panic here? why not return error?




----------------------------------------------------------------
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.

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