Alanxtl commented on code in PR #3395:
URL: https://github.com/apache/dubbo-go/pull/3395#discussion_r3392753362
##########
metadata/client.go:
##########
Review Comment:
URL guard 只覆盖缺 protocol，没有覆盖缺
port。[metadata/client.go](/tmp/dubbo-go-pr3395-review/metadata/client.go:223)
只在 `protocol == ""` 时返回 nil，但
[metadata/client.go](/tmp/dubbo-go-pr3395-review/metadata/client.go:237) 仍会用空
`port` 构造非 nil URL，然后进入 `Refer`，变成下游连接错误。#3351 明确提到要对 URL/protocol/port
缺失给明确错误，这里应该在 URL 层返回 port-missing error，并补测试。
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -258,20 +263,61 @@ func GetMetadataInfo(app string, instance
registry.ServiceInstance, revision str
var metadataInfo *info.MetadataInfo
var err error
if instance.GetMetadata() == nil {
+ // No metadata map at all; treat as default (local/RPC) storage
type.
metadataStorageType = constant.DefaultMetadataStorageType
} else {
metadataStorageType =
instance.GetMetadata()[constant.MetadataStorageTypePropertyName]
+ if metadataStorageType == "" {
+ // MetadataStorageTypePropertyName absent (e.g. old
Java provider); default to local storage type.
+ logger.Warnf("[Metadata] MetadataStorageType not set
for instance %s, defaulting to local", instance.GetID())
+ metadataStorageType =
constant.DefaultMetadataStorageType
+ }
}
+
if metadataStorageType == constant.RemoteMetadataStorageType {
- metadataInfo, err =
metadata.GetMetadataFromMetadataReport(revision, instance, registryId)
- if err != nil {
- return nil, err
+ var reportErr error
+ metadataInfo, reportErr =
metadata.GetMetadataFromMetadataReport(revision, instance, registryId)
+ if reportErr == nil && metadataInfo != nil {
+ metaCache.Set(cacheKey, metadataInfo)
+ return metadataInfo, nil
}
- } else {
+ if reportErr != nil {
+ logger.Errorf("[Metadata-Fallback] report failed,
fallback to RPC app=%s registry=%s revision=%s err=%v",
+ app, registryId, revision, reportErr)
+ } else {
+ logger.Warnf("[Metadata-Fallback] report returned nil
metadata, fallback to RPC app=%s registry=%s revision=%s",
+ app, registryId, revision)
+ }
+
metadataInfo, err = metadata.GetMetadataFromRpc(revision,
instance)
if err != nil {
- return nil, err
+ if reportErr != nil {
+ // Wrap rpcErr so callers can use errors.Is/As
on the primary failure;
+ // reportErr is annotated as context since it
triggered the fallback.
+ return nil, perrors.Wrapf(err,
+ "[Metadata-Fallback] both paths failed,
reportErr: %v", reportErr)
+ }
+ // reportErr was nil — the report returned nil metadata
and RPC also failed.
+ return nil, perrors.Wrapf(err,
+ "[Metadata-Fallback] RPC fallback failed after
report returned nil metadata")
Review Comment:
这怎么return完了还有if 还有return
##########
registry/servicediscovery/service_instances_changed_listener_impl.go:
##########
@@ -258,20 +263,61 @@ func GetMetadataInfo(app string, instance
registry.ServiceInstance, revision str
var metadataInfo *info.MetadataInfo
var err error
if instance.GetMetadata() == nil {
+ // No metadata map at all; treat as default (local/RPC) storage
type.
metadataStorageType = constant.DefaultMetadataStorageType
} else {
metadataStorageType =
instance.GetMetadata()[constant.MetadataStorageTypePropertyName]
+ if metadataStorageType == "" {
+ // MetadataStorageTypePropertyName absent (e.g. old
Java provider); default to local storage type.
+ logger.Warnf("[Metadata] MetadataStorageType not set
for instance %s, defaulting to local", instance.GetID())
+ metadataStorageType =
constant.DefaultMetadataStorageType
+ }
}
+
if metadataStorageType == constant.RemoteMetadataStorageType {
- metadataInfo, err =
metadata.GetMetadataFromMetadataReport(revision, instance, registryId)
- if err != nil {
- return nil, err
+ var reportErr error
+ metadataInfo, reportErr =
metadata.GetMetadataFromMetadataReport(revision, instance, registryId)
+ if reportErr == nil && metadataInfo != nil {
+ metaCache.Set(cacheKey, metadataInfo)
+ return metadataInfo, nil
}
- } else {
+ if reportErr != nil {
+ logger.Errorf("[Metadata-Fallback] report failed,
fallback to RPC app=%s registry=%s revision=%s err=%v",
+ app, registryId, revision, reportErr)
+ } else {
+ logger.Warnf("[Metadata-Fallback] report returned nil
metadata, fallback to RPC app=%s registry=%s revision=%s",
+ app, registryId, revision)
+ }
Review Comment:
这一段的if else 逻辑改成
if reportErr != nil {
} else if metadataInfo == nil {
} else
##########
registry/servicediscovery/service_instances_changed_listener_impl_test.go:
##########
@@ -372,3 +374,216 @@ func (c *capturingNotifyListener) NotifyAll(events
[]*registry.ServiceEvent, cal
callback()
}
}
+
+func TestGetMetadataInfo_CacheKeyFormat(t *testing.T) {
+ // Ensure cache is initialized (normally done by
NewServiceInstancesChangedListener)
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ revision := "rev-cache-key-test"
+ // Pre-populate cache with the expected composite key
+ expectedKey := testApp + ":" + constant.DefaultKey + ":" + revision
+ expectedMeta := newTestMetadataInfo(t, revision, 20000, "dev")
+ metaCache.Set(expectedKey, expectedMeta)
+ t.Cleanup(func() {
+ metaCache.Delete(expectedKey)
+ })
+
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20000",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20000,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName: revision,
+ constant.ServiceInstanceEndpoints:
`[{"port":20000,"protocol":"tri"}]`,
+ },
+ }
+
+ // Should hit the cache and return the pre-populated metadata
+ meta, err := GetMetadataInfo(testApp, instance, revision,
constant.DefaultKey)
+ assert.NoError(t, err)
+ assert.Equal(t, expectedMeta, meta)
+}
+
+func TestGetMetadataInfo_LocalStorageGoesDirectlyToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // Instance with no MetadataStorageTypePropertyName (i.e. local/default
path)
+ // should go directly to RPC without touching the metadata report.
+ // RPC will fail with a URL error because there are no URL params —
that's
+ // enough to confirm the correct branch was taken.
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20003",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20003,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-local-rpc",
+ // MetadataStorageTypePropertyName intentionally absent
→ local path
+ constant.ServiceInstanceEndpoints:
`[{"port":20003,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-local-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Must be a URL/RPC error, not a report error, confirming the local
path
+ // skips the report entirely and goes straight to RPC.
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "local storage path should go directly to RPC, not touch the
metadata report")
+}
+
+func TestGetMetadataInfo_FallbackToRPC(t *testing.T) {
+ // Ensure cache is initialized
+ _ = NewServiceInstancesChangedListener(testApp, constant.DefaultKey,
gxset.NewSet(testApp))
+
+ // remote storage type without a report registered → report will fail
+ // should fall through to RPC, which will fail with url error (no URL
params)
+ instance := ®istry.DefaultServiceInstance{
+ ID: "127.0.0.1:20002",
+ ServiceName: testApp,
+ Host: "127.0.0.1",
+ Port: 20002,
+ Enable: true,
+ Healthy: true,
+ Metadata: map[string]string{
+ constant.ExportedServicesRevisionPropertyName:
"rev-fallback-to-rpc",
+ constant.MetadataStorageTypePropertyName:
constant.RemoteMetadataStorageType,
+ constant.ServiceInstanceEndpoints:
`[{"port":20002,"protocol":"tri"}]`,
+ },
+ }
+
+ _, err := GetMetadataInfo(testApp, instance, "rev-fallback-to-rpc",
constant.DefaultKey)
+ require.Error(t, err)
+ // Both report and RPC fail: error should be a combined Fallback error
containing both causes.
+ // [Metadata-Fallback] prefix proves the fallback path was taken.
+ assert.Contains(t, err.Error(), "[Metadata-Fallback]",
+ "fallback path should produce a [Metadata-Fallback] error")
+ assert.Contains(t, err.Error(), "[Metadata-URL]",
+ "fallback error should include the RPC/URL failure cause")
+}
+
+// TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds verifies the path where
the metadata
+// report returns (nil, nil) — no error but no data — and RPC provides a valid
result.
+// The result should be cached and returned successfully.
+func TestGetMetadataInfo_ReportReturnsNil_RPCSucceeds(t *testing.T) {
+ const regID = "report-nil-rpc-ok"
+ const revision = "rev-report-nil-rpc-ok"
+
+ // Register a mock report that returns (nil, nil) — success with no
data.
+ mockReport := new(listenerMockMetadataReport)
+ extension.SetMetadataReportFactory(regID, func()
metadatareport.MetadataReportFactory {
+ return mockReport
+ })
+ opts := metadata.NewReportOptions(
+ metadata.WithRegistryId(regID),
+ metadata.WithProtocol(regID),
+ metadata.WithAddress("127.0.0.1"),
+ )
+ require.NoError(t, opts.Init())
+ t.Cleanup(metadata.ClearMetadataReportInstances)
+ t.Cleanup(func() {
+ if metaCache != nil {
+ metaCache.Delete(testApp + ":" + regID + ":" + revision)
+ }
+ })
+
Review Comment:
新增测试有两处名字/注释和实际覆盖不一致。[service_instances_changed_listener_impl_test.go](/tmp/dubbo-go-pr3395-review/registry/servicediscovery/service_instances_changed_listener_impl_test.go:473)
叫 `RPCSucceeds`，但实际故意缺 URL params 并断言
error；[service_instances_changed_listener_impl_test.go](/tmp/dubbo-go-pr3395-review/registry/servicediscovery/service_instances_changed_listener_impl_test.go:539)
说验证 RPC 返回 nil metadata，但实际只是 cache fast-path typed nil，没有走 RPC，也没有覆盖生产代码里的
nil guard。建议要么改名和注释，要么用 mock protocol 真正覆盖 fallback success / RPC nil result。
--
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]
