Copilot commented on code in PR #3479:
URL: https://github.com/apache/dubbo-go/pull/3479#discussion_r3536239705
##########
registry/nacos/registry.go:
##########
@@ -375,9 +386,42 @@ func (nr *nacosRegistry) LoadSubscribeInstances(url
*common.URL, notify registry
return nil
}
+func (nr *nacosRegistry) storeInitialSubscribeInstances(serviceName string,
groupName string, instances []model.Instance) {
+ if len(instances) == 0 {
+ return
+ }
+ copied := make([]model.Instance, len(instances))
+ copy(copied, instances)
+ nr.initialSubscribeInstances.Store(subscribeCacheKey(serviceName,
groupName), copied)
+}
+
+func (nr *nacosRegistry) loadInitialSubscribeInstances(serviceName string,
groupName string) ([]model.Instance, bool) {
+ value, ok :=
nr.initialSubscribeInstances.Load(subscribeCacheKey(serviceName, groupName))
+ if !ok {
+ return nil, false
+ }
+ instances, ok := value.([]model.Instance)
+ return instances, ok
+}
+
+func (nr *nacosRegistry) deleteInitialSubscribeInstances(serviceName string,
groupName string) {
+ nr.initialSubscribeInstances.Delete(subscribeCacheKey(serviceName,
groupName))
+}
+
+func (nr *nacosRegistry) clearInitialSubscribeInstances() {
+ nr.initialSubscribeInstances.Range(func(key any, _ any) bool {
+ nr.initialSubscribeInstances.Delete(key)
+ return true
+ })
+}
+
+func subscribeCacheKey(serviceName string, groupName string) string {
+ return serviceName + groupName
+}
Review Comment:
subscribeCacheKey currently builds the cache key by concatenating
serviceName and groupName directly. This can produce ambiguous keys (e.g.,
serviceName="ab", groupName="c" vs serviceName="a", groupName="bc"), which can
cause listenerCache/initialSubscribeInstances lookups to collide across
different subscriptions.
##########
registry/nacos/registry.go:
##########
@@ -279,13 +281,18 @@ func (nr *nacosRegistry) subscribe(serviceName string,
notifyListener registry.N
return perrors.New("nacosRegistry is not available.")
}
listener := NewNacosListenerWithServiceName(serviceName, nr.URL,
nr.namingClient)
+ groupName := nr.GetParam(constant.RegistryGroupKey, defaultGroup)
+ if instances, ok := nr.loadInitialSubscribeInstances(serviceName,
groupName); ok {
+ listener.setInstanceSnapshot(instances)
+ }
Review Comment:
The new initial snapshot seeding (LoadSubscribeInstances ->
storeInitialSubscribeInstances -> setInstanceSnapshot) changes subscription
semantics and is described in the PR as having a regression test, but there is
no coverage in the nacos registry tests for the stale-initial-provider scenario
(initial A loaded, then first push B should emit add(B) + del(A)). Please add a
unit/regression test to prevent this from regressing.
--
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]
