marsevilspirit commented on code in PR #2859:
URL: https://github.com/apache/dubbo-go/pull/2859#discussion_r2072704825
##########
registry/nacos/registry.go:
##########
@@ -177,46 +177,67 @@ func (nr *nacosRegistry) Subscribe(url *common.URL,
notifyListener registry.Noti
return nil
}
serviceName := url.GetParam(constant.InterfaceKey, "")
- var serviceNames []string
- var err error
if serviceName == constant.AnyValue {
- serviceNames, err = nr.getAllSubscribeServiceNames(url)
- if err != nil {
- return err
- }
+ nr.subscribeAll(url, notifyListener)
+ go func() {
Review Comment:
这里的goroutine也像下面的subscribe一样封装一下吧，将goroutine封装到subscribeAll函数中，统一一下风格。
##########
registry/nacos/registry.go:
##########
@@ -177,46 +177,67 @@ func (nr *nacosRegistry) Subscribe(url *common.URL,
notifyListener registry.Noti
return nil
}
serviceName := url.GetParam(constant.InterfaceKey, "")
- var serviceNames []string
- var err error
if serviceName == constant.AnyValue {
- serviceNames, err = nr.getAllSubscribeServiceNames(url)
- if err != nil {
- return err
- }
+ nr.subscribeAll(url, notifyListener)
+ go func() {
+ // scheduled lookup for new service
+ for {
+ nr.subscribeAll(url, notifyListener)
+ time.Sleep(LookupInterval)
+ }
+ }()
+ return nil
} else {
- serviceNames = []string{getSubscribeName(url)}
+ // retry forever
+ for {
+ err := nr.subscribe(getSubscribeName(url),
notifyListener)
+ if err == nil {
+ return nil
+ }
+ }
}
- return nr.subscribe(serviceNames, notifyListener)
}
-// subscribe subscribe services
-func (nr *nacosRegistry) subscribe(serviceNames []string, notifyListener
registry.NotifyListener) error {
+func (nr *nacosRegistry) subscribeAll(url *common.URL, notifyListener
registry.NotifyListener) {
+ groupName := nr.URL.GetParam(constant.RegistryGroupKey, defaultGroup)
+ serviceNames, err := nr.getAllSubscribeServiceNames(url)
+ if err != nil {
+ logger.Warnf("getAllServices() = err:%v",
perrors.WithStack(err))
+ return
+ }
if len(serviceNames) == 0 {
logger.Warnf("No services to listen to.")
- return nil
+ return
}
- for {
- if !nr.IsAvailable() {
- logger.Warnf("event listener game over.")
- return perrors.New("nacosRegistry is not available.")
+ for _, name := range serviceNames {
+ if _, ok := listenerCache.Load(name + groupName); ok {
+ continue
}
- var err error
- for _, serviceName := range serviceNames {
- listener :=
NewNacosListenerWithServiceName(serviceName, nr.URL, nr.namingClient)
- err = listener.listenService(serviceName)
- metrics.Publish(metricsRegistry.NewSubscribeEvent(err
== nil))
- if err != nil {
- logger.Warnf("getAllServices() = err:%v",
perrors.WithStack(err))
- time.Sleep(time.Duration(RegistryConnDelay) *
time.Second)
- break
- }
- go nr.handleServiceEvents(listener, notifyListener)
- }
- if err == nil {
- break
+ err = nr.subscribe(name, notifyListener)
+ if err != nil {
+ logger.Warnf("subscribe service %s err:%v", name,
perrors.WithStack(err))
}
}
+}
+
+// subscribe subscribe services
+func (nr *nacosRegistry) subscribe(serviceName string, notifyListener
registry.NotifyListener) error {
+ if len(serviceName) == 0 {
+ logger.Warnf("can not subscribe because service name is empty")
+ return nil
+ }
+ if !nr.IsAvailable() {
+ logger.Warnf("event listener game over.")
+ return perrors.New("nacosRegistry is not available.")
+ }
+ listener := NewNacosListenerWithServiceName(serviceName, nr.URL,
nr.namingClient)
+ err := listener.listenService(serviceName)
+ metrics.Publish(metricsRegistry.NewSubscribeEvent(err == nil))
+ if err != nil {
+ logger.Warnf("subscribe service %s err:%v", serviceName,
perrors.WithStack(err))
+ return err
+ }
+ go nr.handleServiceEvents(listener, notifyListener)
Review Comment:
这样确实会有`不受控制的 gr`的问题，但是目前也没有必要去控制这个goroutine，事实上这个goroutine有从始至终存在的必要。
`不受控制的 gr`应该是担心goroutine泄漏，这里应该不会出现这个问题吧？
##########
registry/nacos/listener.go:
##########
@@ -176,7 +176,7 @@ func (nl *nacosListener) listenService(serviceName string)
error {
if nl.namingClient == nil {
return perrors.New("nacos naming namingClient stopped")
}
- nl.subscribeParam = createSubscribeParam(serviceName, nl.regURL,
nl.Callback)
+ nl.subscribeParam = createSubscribeParam(serviceName,
nl.regURL.GetParam(constant.RegistryGroupKey, defaultGroup), nl.Callback)
Review Comment:
这行代码太长了，要不然分成两行写吧，更清晰
```go
groupName := nl.regURL.GetParam(constant.RegistryGroupKey, defaultGroup)
nl.subscribeParam = createSubscribeParam(serviceName, groupName, nl.Callback)
```
##########
registry/nacos/registry.go:
##########
@@ -177,46 +177,67 @@ func (nr *nacosRegistry) Subscribe(url *common.URL,
notifyListener registry.Noti
return nil
}
serviceName := url.GetParam(constant.InterfaceKey, "")
- var serviceNames []string
- var err error
if serviceName == constant.AnyValue {
- serviceNames, err = nr.getAllSubscribeServiceNames(url)
- if err != nil {
- return err
- }
+ nr.subscribeAll(url, notifyListener)
+ go func() {
+ // scheduled lookup for new service
+ for {
+ nr.subscribeAll(url, notifyListener)
+ time.Sleep(LookupInterval)
+ }
+ }()
+ return nil
} else {
- serviceNames = []string{getSubscribeName(url)}
+ // retry forever
+ for {
Review Comment:
这两个for循环可以放在函数里面，if else嵌套在套上for循环可读性不高。
放在函数里，加一些函数的comment说明一下启动了goroutine就好了。
##########
registry/nacos/registry.go:
##########
@@ -177,46 +177,67 @@ func (nr *nacosRegistry) Subscribe(url *common.URL,
notifyListener registry.Noti
return nil
}
serviceName := url.GetParam(constant.InterfaceKey, "")
- var serviceNames []string
- var err error
if serviceName == constant.AnyValue {
- serviceNames, err = nr.getAllSubscribeServiceNames(url)
- if err != nil {
- return err
- }
+ nr.subscribeAll(url, notifyListener)
+ go func() {
+ // scheduled lookup for new service
+ for {
+ nr.subscribeAll(url, notifyListener)
+ time.Sleep(LookupInterval)
Review Comment:
我也觉得没有特殊需求的话，先这样没有问题，就是这个10s的间隔不太好把握，在这里留一个TODO吧
##########
registry/nacos/listener.go:
##########
@@ -176,7 +176,7 @@ func (nl *nacosListener) listenService(serviceName string)
error {
if nl.namingClient == nil {
return perrors.New("nacos naming namingClient stopped")
}
- nl.subscribeParam = createSubscribeParam(serviceName, nl.regURL,
nl.Callback)
+ nl.subscribeParam = createSubscribeParam(serviceName,
nl.regURL.GetParam(constant.RegistryGroupKey, defaultGroup), nl.Callback)
Review Comment:
这个url跟context一样，最让人头疼的地方就是取出参数的时机，
这种提前取出参数的方式我认为完全ok，增加了可读性，就是取参数时要是能统一写在一起就好看多了。
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
