AlexStocks commented on code in PR #3232:
URL: https://github.com/apache/dubbo-go/pull/3232#discussion_r2938126728
##########
client/options.go:
##########
@@ -447,14 +422,15 @@ func WithMeshProviderPort(port int) ReferenceOption {
}
}
-func WithMethod(opts ...config.MethodOption) ReferenceOption {
- regOpts := config.NewMethodOptions(opts...)
-
+func WithMethod(method *global.MethodConfig) ReferenceOption {
Review Comment:
确认收到。看到 server/options.go 也同步加了 `WithMethod(method
*global.MethodConfig)`，client 和 server 端签名一致，这点做得好。
不过 PR 描述里建议加一段 migration guide，说明老调用方怎么迁移，比如：
```go
// Before (v3.x)
WithMethod(config.WithName("foo"), config.WithRetries(3))
// After (v3.y)
WithMethod(&global.MethodConfig{Name: "foo", Retries: "3"})
```
方便升级的用户快速定位改法。
##########
client/action.go:
##########
@@ -242,7 +242,7 @@ func processURL(ref *global.ReferenceConfig, regsCompat
map[string]*config.Regis
}
}
} else { // use registry configs
- urls = config.LoadRegistries(ref.RegistryIDs, regsCompat,
common.CONSUMER)
+ urls = internal.LoadRegistries(ref.RegistryIDs, registries,
common.CONSUMER)
Review Comment:
修复确认 ✓
`LoadRegistries` 签名改为 `([]*common.URL, error)`，非法 URL 和缺失 registry ID 都返回
error 而不是静默跳过，client/action.go 和 server/action.go 都正确传播了错误。`loadRegistryURLs`
还加了 `len(urls) == 0` 的防御性检查和 `buildInvoker` 的空 urls 防护，都很好。
测试覆盖完整：`TestLoadRegistries_InvalidURL`、`TestLoadRegistries_MissingRegistryID`、`TestProcessURLRejectsEmptyRegistryURLs`、`TestExportReturnsRegistryLoadError`。LGTM。
##########
client/options.go:
##########
@@ -585,35 +547,24 @@ func (cliOpts *ClientOptions) init(opts ...ClientOption)
error {
consumerConf := cliOpts.Consumer
- // init application
- application := cliOpts.Application
- if application != nil {
- cliOpts.applicationCompat = compatApplicationConfig(application)
- if err := cliOpts.applicationCompat.Init(); err != nil {
- return err
- }
- }
-
// init registries
regs := cliOpts.Registries
- if regs != nil {
- cliOpts.registriesCompat =
make(map[string]*config.RegistryConfig)
+ if len(regs) > 0 {
if len(consumerConf.RegistryIDs) <= 0 {
- consumerConf.RegistryIDs = make([]string, len(regs))
+ consumerConf.RegistryIDs = make([]string, 0, len(regs))
for key := range regs {
consumerConf.RegistryIDs =
append(consumerConf.RegistryIDs, key)
}
}
consumerConf.RegistryIDs =
commonCfg.TranslateIds(consumerConf.RegistryIDs)
+ newRegs := make(map[string]*global.RegistryConfig)
for _, id := range consumerConf.RegistryIDs {
if reg, ok := regs[id]; ok {
- cliOpts.registriesCompat[id] =
compatRegistryConfig(reg)
- if err := cliOpts.registriesCompat[id].Init();
err != nil {
- return err
- }
+ newRegs[id] = reg
}
}
+ cliOpts.Registries = newRegs
Review Comment:
修复确认 ✓
做法比我原来建议的更好：直接去掉了 `registriesCompat` 中间层，`init()` 不再做过滤和替换，只做
`validateRegistryIDs` 校验。真正的按需过滤放到了 `dial()` 阶段的 `filterRegistriesByIDs()`，返回新
map 而不修改原始字段。
`TestClientOptionsInitKeepsOriginalRegistries` 验证了 init 后 Registries
长度不变。逻辑清晰，LGTM。
--
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]
