AlexStocks commented on code in PR #982: URL: https://github.com/apache/dubbo-go-pixiu/pull/982#discussion_r3594722861
########## pkg/adapter/dubboregistry/registry/nacos/registry.go: ########## @@ -18,10 +18,10 @@ package nacos import ( - "github.com/nacos-group/nacos-sdk-go/clients" - "github.com/nacos-group/nacos-sdk-go/clients/naming_client" - nacosConstant "github.com/nacos-group/nacos-sdk-go/common/constant" - "github.com/nacos-group/nacos-sdk-go/vo" + "github.com/nacos-group/nacos-sdk-go/v2/clients" Review Comment: [P1] 补齐 Nacos 1.x 与 gRPC 端口的升级门禁 这不是纯 API 替换。Nacos 官方兼容说明明确：2.x 客户端不兼容 1.x 服务端，并且客户端会通过主端口 +1000（默认 8848 → 9848）建立 gRPC；存在防火墙、容器映射或代理时必须额外开放该 TCP 端口。当前仓库的 Nacos 配置/文档仍只要求 8848，`docs/ai/registry*.md` 甚至仍示例导入 v1 `vo`，也没有声明最低服务端版本，因此现有 Nacos 1.x 或只开放 8848 的部署会在升级后运行期失联。请保留兼容路径，或明确最低 Nacos 2.x、补充 9848/TCP 转发与升级文档，并增加真实 Nacos 2.x 的连接验证。官方依据：https://github.com/nacos-group/nacos-group.github.io/blob/5e9d9ca33caeac97ebef7146f6f3f2b3a82793f6/src/content/docs/v2/en/upgrading/200-compatibility.md ########## pkg/adapter/llmregistry/registry/nacos/listener_test.go: ########## @@ -77,6 +77,12 @@ func (m *mockNacosClient) Unsubscribe(param *vo.SubscribeParam) error { return nil } +func (m *mockNacosClient) ServerHealthy() bool { + return true +} + +func (m *mockNacosClient) CloseClient() {} Review Comment: [P1] 生产关闭路径没有消费 v2 的 `CloseClient` 这里为满足 v2 `INamingClient` 补了空实现，但生产生命周期仍只做 listener/unsubscribe：LLM `DoUnsubscribe`、Spring Cloud 的 Nacos 包装器以及配置中心都没有调用 `CloseClient()`。v2 的实现会在该方法中执行 gRPC `Shutdown()` 并取消内部 context；仓库现有 MCP v2 适配器也在关闭时显式关闭 naming/config client。当前遗漏会让 Stop/Apply 或优雅退出后的连接、重试协程继续存活。请在所有本次迁移的客户端创建点补齐可达的关闭接口，在取消订阅后调用它，并用 mock 断言关闭发生。 ########## pkg/adapter/springcloud/servicediscovery/nacos/nacos_test.go: ########## @@ -0,0 +1,183 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package nacos + +import ( + "testing" +) + +import ( + "github.com/nacos-group/nacos-sdk-go/v2/model" + + "github.com/stretchr/testify/assert" +) + +import ( + "github.com/apache/dubbo-go-pixiu/pkg/adapter/springcloud/servicediscovery" +) + +func TestCallback_ServiceNameWithGroupPrefix(t *testing.T) { + // Test the @@ prefix stripping logic in Callback + // This tests the new code path where ServiceName contains "DEFAULT_GROUP@@service-name" + + tests := []struct { + name string + serviceName string + expected string + }{ + { + name: "with group prefix", + serviceName: "DEFAULT_GROUP@@user-service", + expected: "user-service", + }, + { + name: "without group prefix", + serviceName: "user-service", + expected: "user-service", + }, + { + name: "empty string", + serviceName: "", + expected: "", + }, + { + name: "multiple @@ separators", + serviceName: "GROUP@@SUBGROUP@@service", + expected: "SUBGROUP@@service", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Simulate the strings.Cut logic used in Callback + serviceName := tt.serviceName + if _, after, ok := cutServiceName(serviceName, "@@"); ok { + serviceName = after + } + assert.Equal(t, tt.expected, serviceName) Review Comment: [P2] 这条测试没有执行生产 `Callback` 当前用测试文件里的 `cutServiceName` 复制了一遍分隔逻辑，生产代码即使删除前缀处理、改错分隔符或没有把结果传给 listener，这条测试仍会通过。请让 mock 记录收到的 `ServiceInstance`，直接调用 `nacosServiceDiscovery.Callback` 并断言 `ServiceName`，然后删除这份复制实现。 -- 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]
