Alanxtl commented on code in PR #3373:
URL: https://github.com/apache/dubbo-go/pull/3373#discussion_r3368861526
##########
metadata/report/nacos/report.go:
##########
@@ -165,30 +162,42 @@ func (n *nacosMetadataReport)
RegisterServiceAppMapping(key string, group string
DataId: key,
Group: group,
})
- if oldVal != "" {
- oldApps := strings.Split(oldVal, constant.CommaSeparator)
- if len(oldApps) > 0 {
- for _, app := range oldApps {
- if app == value {
- return nil
- }
- }
- }
- value = oldVal + constant.CommaSeparator + value
+ merged, changed := report.MergeServiceAppMapping(oldVal, value)
+ if !changed {
+ return nil
}
- return n.storeMetadata(vo.ConfigParam{
+ param := vo.ConfigParam{
DataId: key,
Group: group,
- Content: value,
- })
+ Content: merged,
+ }
+ if oldVal != "" {
+ // CasMd5 is an optimistic UPDATE: Nacos publishes only if the
server content still
+ // matches what we read, detecting concurrent appends. It
cannot guard the first INSERT
+ // (Nacos has no create-if-absent), so the initial concurrent
registration of a
+ // brand-new interface can still race. This is a known
Nacos-only limitation; the
+ // etcd and zookeeper reports do not have it.
+ param.CasMd5 = fmt.Sprintf("%x", md5.Sum([]byte(oldVal)))
+ }
+ if err := n.storeMetadata(param); err != nil {
+ if param.CasMd5 != "" {
+ // Nacos surfaces a CAS rejection and a transport error
the same way, so they
+ // cannot be told apart here. Treat the failure as a
retriable conflict rather
+ // than risk dropping a real concurrent update; the
underlying error is preserved
+ // for diagnosis.
+ return fmt.Errorf("publish mapping %s (%v): %w", key,
err, report.ErrMappingCASConflict)
+ }
+ return err
+ }
+ return nil
}
Review Comment:
Nacos 读失败会被当成空旧值继续写，可能覆盖已有 mapping。
这里 `oldVal, _ := n.getConfig(...)` 仍然吞掉错误。若 Nacos 读因为网络/权限/服务端异常失败，但 key
实际已有 `appA,appB`，这里会按 `oldVal == ""` 生成 `Content: appC` 且不带
`CasMd5`，最后可能无条件覆盖旧集合。这个 bug 在 base 已存在，但这个 PR 的核心就是“mapping
consistency”，所以这里必须顺手修掉：`getConfig` error 应该返回或进入可控重试，不能继续 publish。
另外我们要把md5算法换掉[#3370](https://github.com/apache/dubbo-go/pull/3370)
casmd5会不会受到影响
##########
metadata/report/zookeeper/listener.go:
##########
@@ -116,6 +116,15 @@ func (l *CacheListener) RemoveListener(key string,
listener mapping.MappingListe
}
}
+// RemoveKeyListeners drops all listeners registered for key so its mapping
change events stop
+// being dispatched. The dispatcher goroutine is shared by the whole mapping
group and is kept
+// alive (other keys still need it); it is released when the report is closed.
The key's
+// underlying ZooKeeper watch is not unregistered here, as ZkEventListener
exposes no per-path
+// unlisten, so the server may keep sending now-ignored events for the key.
+func (l *CacheListener) RemoveKeyListeners(key string) {
+ l.keyListeners.Delete(key)
+}
Review Comment:
Zookeeper listener 路径没有复用新的 decode helper，事件里仍可能带空 app。
129行func (l *CacheListener) DataChange(event remoting.Event) bool {
仍然直接 strings.Split 后全量 set.Add(e)。如果已有 legacy 值是 ,app、app,,other
这类格式，GetServiceAppMapping 会过滤空元素，但 ZK change event 会把 "" 发给 consumer。建议这里也改成
report.DecodeServiceAppNames(event.Content)，并补 listener event 测试。
--
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]
