Copilot commented on code in PR #3371:
URL: https://github.com/apache/dubbo-go/pull/3371#discussion_r3367021937
##########
metadata/report/zookeeper/report.go:
##########
@@ -82,13 +108,48 @@ func (m *zookeeperMetadataReport)
PublishAppMetadata(application, revision strin
return err
}
err = m.client.CreateWithValue(k, data)
- if perrors.Is(err, zk.ErrNodeExists) {
- logger.Debug("[Metadata][Zookeeper] try to create the node data
failed. In most cases, it's not a problem. ")
+ if err == zk.ErrNodeExists {
+ _, err = m.client.SetContent(k, data, -1)
+ }
Review Comment:
`CreateWithValue` may return a wrapped `zk.ErrNodeExists` (other codepaths
in this repo handle it via `perrors.Is`). Using direct equality here risks
skipping the update path and leaving existing app-metadata nodes stale when
republishing.
Consider switching to `errors.Is(err, zk.ErrNodeExists)` (or `perrors.Is`),
and similarly for `zk.ErrNoNode` checks if the client wraps errors.
##########
metadata/report/nacos/report.go:
##########
@@ -214,6 +214,70 @@ func (n *nacosMetadataReport)
RemoveServiceAppMappingListener(key string, group
return n.removeServiceMappingListener(key, group)
}
+// UnPublishAppMetadata removes metadata for a specific revision from nacos.
+// This operation is idempotent — deleting a non-existent config returns false
but no error.
+func (n *nacosMetadataReport) UnPublishAppMetadata(application, revision
string) error {
+ // Delete primary config (compatible with java impl)
+ _, err := n.client.Client().DeleteConfig(vo.ConfigParam{
+ DataId: application,
+ Group: revision,
+ })
+ if err != nil {
+ return perrors.WithMessage(err, "Could not delete the metadata")
+ }
+ // Delete legacy config (compatible with dubbo-go 3.1.x).
+ if _, err = n.client.Client().DeleteConfig(vo.ConfigParam{
+ DataId: application + constant.KeySeparator + revision,
+ Group: n.group,
+ }); err != nil {
+ logger.Warnf("[Metadata][Nacos] could not delete legacy
metadata for app=%s rev=%s: %v",
+ application, revision, err)
+ }
+ return nil
+}
+
+// ListAppRevisions lists all stored revisions for an application from nacos.
+func (n *nacosMetadataReport) ListAppRevisions(application string)
([]report.AppRevision, error) {
+ pageNo, pageSize := 1, 500
+ configs, err := n.client.Client().SearchConfig(vo.SearchConfigParam{
+ Search: "accurate",
+ DataId: application,
+ Group: "",
+ PageNo: pageNo,
+ PageSize: pageSize,
+ })
Review Comment:
`ListAppRevisions` only fetches the first page (500) and then proceeds with
a partial set of revisions. This can make GC incomplete for long-lived apps
(revisions beyond the first page will never be considered for cleanup). The
warning log mentions silent drops, but GC correctness still depends on seeing
all revisions.
##########
registry/servicediscovery/service_discovery_registry.go:
##########
@@ -298,12 +320,142 @@ func (s *serviceDiscoveryRegistry) IsAvailable() bool {
}
func (s *serviceDiscoveryRegistry) Destroy() {
+ s.stopMetadataTimers()
err := s.serviceDiscovery.Destroy()
if err != nil {
logger.Errorf("[Registry][ServiceDiscovery] destroy
serviceDiscovery catch error, err=%s", err.Error())
}
}
+func (s *serviceDiscoveryRegistry) stopMetadataTimers() {
+ s.lock.Lock()
+ defer s.lock.Unlock()
+ if s.renewAppMetadataTimer != nil {
+ s.renewAppMetadataTimer.Stop()
+ s.renewAppMetadataTimer = nil
+ }
+}
+
+// ========== renewAppMetadata: daily app-level metadata re-publish ==========
+
+func (s *serviceDiscoveryRegistry) startRenewAppMetadataTimer() {
+ if !s.url.GetParamBool(constant.CycleReportKey, true) {
+ return
+ }
+
+ // Run immediately on start
+ if s.url.GetParamBool(constant.MetadataRenewOnStartupKey, true) {
+ go s.doRenewAppMetadata()
+ }
+
+ delay := s.calculateRenewAppMetadataDelay()
+ s.renewAppMetadataTimer = time.AfterFunc(delay, func() {
+ s.doRenewAppMetadata()
+ // Reschedule for next day
+ s.lock.Lock()
+ if s.renewAppMetadataTimer != nil {
+ s.renewAppMetadataTimer.Reset(24 * time.Hour)
+ }
+ s.lock.Unlock()
+ })
+}
+
+func (s *serviceDiscoveryRegistry) doRenewAppMetadata() {
+ registryID := s.url.GetParam(constant.RegistryIdKey, "")
+ metaInfo := metadata.GetMetadataInfo(registryID)
+ if metaInfo == nil || metaInfo.Revision == "0" {
+ return
+ }
+ metaInfo.LastUpdatedTime = time.Now().UnixMilli()
+ if err := s.metadataReport.PublishAppMetadata(metaInfo.App,
metaInfo.Revision, metaInfo); err != nil {
+ logger.Errorf("[Metadata][renewAppMetadata] failed to
re-publish metadata for app=%s revision=%s: %v", metaInfo.App,
metaInfo.Revision, err)
+ } else {
+ logger.Infof("[Metadata][renewAppMetadata] refreshed metadata
for app=%s revision=%s", metaInfo.App, metaInfo.Revision)
+ }
+
+ // Run garbage collection if enabled, after each renew cycle
+ if s.url.GetParamBool(constant.MetadataGCEnabledKey, true) {
+ s.doGarbageCollect()
+ }
+}
+
+func (s *serviceDiscoveryRegistry) calculateRenewAppMetadataDelay()
time.Duration {
+ now := time.Now()
+ // Next day 2:00 AM
+ nextDay2AM := time.Date(now.Year(), now.Month(), now.Day()+1, 2, 0, 0,
0, now.Location())
+ // Add random offset 0~4 hours to avoid thundering herd
+ randomOffset := time.Duration(rand.Int64N(int64(4 * time.Hour)))
+ return time.Until(nextDay2AM) + randomOffset
+}
+
+// ========== GC: stale revision cleanup ==========
+
+func (s *serviceDiscoveryRegistry) doGarbageCollect() {
+ registryID := s.url.GetParam(constant.RegistryIdKey, "")
+ metaInfo := metadata.GetMetadataInfo(registryID)
+ if metaInfo == nil {
+ return
+ }
+ app := metaInfo.App
+ if app == "" {
+ return
+ }
+
+ // Step 1: List all revisions for this app
+ revisions, err := s.metadataReport.ListAppRevisions(app)
+ if err != nil {
+ logger.Warnf("[Metadata][GC] failed to list app revisions: %v",
err)
+ return
+ }
+ if len(revisions) == 0 {
+ return
+ }
+
+ // Step 2: Filter stale candidates (exceed GC window in days)
+ gcWindowDays := s.url.GetParamByIntValue(constant.MetadataGCWindowKey,
5)
+ if gcWindowDays <= 0 || gcWindowDays > 365 {
+ gcWindowDays = 5
+ }
+ cutoff := time.Now().AddDate(0, 0, -gcWindowDays).UnixMilli()
+ candidates := make(map[string]bool)
+ for _, rev := range revisions {
+ // Skip special revisions
+ if rev.Revision == "0" || rev.Revision == "N/A" || rev.Revision
== "" || rev.Revision == metaInfo.Revision {
+ continue
+ }
+ // ModifyTime == 0 means old metadata produced by a version
that does not set
+ // lastUpdatedTime — never garbage-collect such entries. Only
delete when the
+ // revision is older than gcWindow and no alive instance
references it.
Review Comment:
The comment describes deleting `ModifyTime == 0` entries when they are older
than the GC window, but the current logic never adds `ModifyTime == 0`
revisions to GC candidates. This is confusing for readers and makes it harder
to reason about the actual cleanup semantics.
##########
metadata/report/etcd/report.go:
##########
@@ -111,6 +128,39 @@ func (e *etcdMetadataReport)
RemoveServiceAppMappingListener(key string, group s
return nil
}
+// UnPublishAppMetadata removes metadata for a specific revision from etcd.
+// This operation is idempotent.
+func (e *etcdMetadataReport) UnPublishAppMetadata(application, revision
string) error {
+ key := e.rootDir + constant.PathSeparator + application +
constant.PathSeparator + revision
+ return e.client.Delete(key)
+}
+
+func (e *etcdMetadataReport) ListAppRevisions(application string)
([]report.AppRevision, error) {
+ prefix := e.rootDir + constant.PathSeparator + application +
constant.PathSeparator
+ keys, _, err := e.client.GetChildren(prefix)
+ if err != nil {
+ if perrors.Cause(err) == gxetcd.ErrKVPairNotFound {
+ return nil, nil
+ }
+ return nil, err
+ }
+
+ result := make([]report.AppRevision, 0, len(keys))
+ for _, key := range keys {
+ // Extract revision from key suffix (key is full path, revision
is last segment)
+ revision := key[strings.LastIndex(key,
constant.PathSeparator)+1:]
+ val, err := e.client.Get(key)
+ if err != nil {
+ continue // skip if key disappeared between listing and
reading
+ }
+ result = append(result, report.AppRevision{
+ Revision: revision,
+ ModifyTime:
report.ParseMetadataLastUpdatedTime([]byte(val)),
+ })
+ }
+ return result, nil
+}
Review Comment:
`ListAppRevisions` already receives both keys and values from `GetChildren`,
but it discards the values and performs an extra `Get` per key. In real etcd
this becomes an N+1 network round-trip pattern and can noticeably slow down the
daily GC pass for apps with many revisions.
##########
metadata/report/zookeeper/report_test.go:
##########
@@ -34,6 +36,267 @@ import (
"dubbo.apache.org/dubbo-go/v3/metadata/info"
)
+// --- Mock zkClient ---
+// mockZkClient implements zkClient for testing.
+type mockZkClient struct {
+ data map[string][]byte // path -> value
+ stats map[string]*zk.Stat // path -> stat
+ errors map[string]error // path -> error for specific operations
(optional override)
+}
+
+func newMockZkClient() *mockZkClient {
+ return &mockZkClient{
+ data: make(map[string][]byte),
+ stats: make(map[string]*zk.Stat),
+ errors: make(map[string]error),
+ }
+}
+
+func (m *mockZkClient) GetContent(path string) ([]byte, *zk.Stat, error) {
+ if err, ok := m.errors["GetContent:"+path]; ok {
+ return nil, nil, err
+ }
+ v, ok := m.data[path]
+ if !ok {
+ return nil, nil, zk.ErrNoNode
+ }
+ return v, m.stats[path], nil
+}
+
+func (m *mockZkClient) SetContent(path string, data []byte, version int32)
(*zk.Stat, error) {
+ if err, ok := m.errors["SetContent:"+path]; ok {
+ return nil, err
+ }
+ m.data[path] = data
+ stat := &zk.Stat{Version: version + 1, Mtime: int64(version + 1)}
+ m.stats[path] = stat
+ return stat, nil
+}
+
+func (m *mockZkClient) CreateWithValue(path string, data []byte) error {
+ if err, ok := m.errors["CreateWithValue:"+path]; ok {
+ return err
+ }
+ m.data[path] = data
+ stat := &zk.Stat{Version: 0, Mtime: 0}
+ m.stats[path] = stat
+ return nil
+}
Review Comment:
The new zookeeper update behavior (create-or-update on `ErrNodeExists`)
isn't actually exercised by the tests because this mock `CreateWithValue` never
returns `zk.ErrNodeExists` for existing paths. As a result,
`TestPublishAppMetadata_Update` can pass even if the production update branch
is broken.
--
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]
