Skip to content

remove the config package from package client#3232

Open
nanjiek wants to merge 1 commit intoapache:developfrom
nanjiek:last_version
Open

remove the config package from package client#3232
nanjiek wants to merge 1 commit intoapache:developfrom
nanjiek:last_version

Conversation

@nanjiek
Copy link
Contributor

@nanjiek nanjiek commented Mar 7, 2026

Description

Fixes #3204

Completed the final core config removal in the client package, and refactored options_test based on usage scenarios.

Checklist

  • I confirm the target branch is develop
  • Code has passed local testing
  • I have added tests that prove my fix is effective or that my feature works

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2026

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 35.29412% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.96%. Comparing base (60d1c2a) to head (18c244e).
⚠️ Report is 750 commits behind head on develop.

Files with missing lines Patch % Lines
client/options.go 35.29% 10 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3232      +/-   ##
===========================================
+ Coverage    46.76%   47.96%   +1.19%     
===========================================
  Files          295      462     +167     
  Lines        17172    33766   +16594     
===========================================
+ Hits          8031    16196    +8165     
- Misses        8287    16264    +7977     
- Partials       854     1306     +452     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Alanxtl Alanxtl linked an issue Mar 8, 2026 that may be closed by this pull request
@Alanxtl Alanxtl added 🧹 Updates 3.3.2 version 3.3.2 labels Mar 8, 2026
Copy link
Contributor

@Alanxtl Alanxtl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

}
}
if refConf.MethodsConfig == nil {
refConf.MethodsConfig = make([]*global.MethodConfig, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

原代码对每个 MethodConfig 调用 config.MethodConfig.Init(),执行 defaults.Set + TpsLimitStrategy/TpsLimitInterval/TpsLimitRate 合法性校验(负数检查、extension 注册检查)。新代码把这些校验全部删掉,只初始化空 slice。

用户配置了无效的 TpsLimitStrategy 或负数 TpsLimitRate,现在要等到运行时在 filter 层才崩溃,原本启动时就能 fail-fast 的问题被延后了。

建议:在 global.MethodConfig 中添加 Validate() 方法,并在 ReferenceOptions.init() 中调用。

func WithMethod(opts ...config.MethodOption) ReferenceOption {
regOpts := config.NewMethodOptions(opts...)

func WithMethod(method *global.MethodConfig) ReferenceOption {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这是破坏性 API 变更。原来是 functional options 风格(WithMethod(config.WithName("foo"), config.WithRetries(3))),现在变成直接传 struct 指针,外部调用方升级后直接编译失败。

PR 描述没有标注 breaking change,也没有 migration guide 或 deprecation 期。

建议:保留旧签名做 adapter 转换为新格式;或在 changelog 和 PR 说明中明确标注 breaking change,并同步处理 server/options.go 中对应的 WithMethod

}
} else { // use registry configs
urls = config.LoadRegistries(ref.RegistryIDs, regsCompat, common.CONSUMER)
urls = internal.LoadRegistries(ref.RegistryIDs, registries, common.CONSUMER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

原来 config.LoadRegistries 在 URL 构建失败时直接 panic(fail-fast)。新的 internal.LoadRegistries 只打印 error log 然后 continue,静默丢弃配置错误的 registry。

用户配置了错误的 registry address,服务启动成功但实际没连上 registry,服务发现静默失效,难以排查。

建议:将 continue 改为返回 error 向上传播;或在所有 registry 均失败时返回错误而不是空列表。processURL 签名已返回 error,可以直接传播。

newRegs[id] = reg
}
}
cliOpts.Registries = newRegs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientOptions.init() 直接把 cliOpts.Registries 替换为过滤后的新 map,原始 map 被永久丢弃。用户在外部持有同一个 ClientOptions 指针时,init() 之后看到的 .Registries 是过滤后的结果,与传入值不一致,调用方对此无感知。

建议:在文档注释中说明 init() 会修改此字段;或改为在 dial() 时做临时过滤,不修改 cliOpts 的持久状态。

}
}
refOpts.registriesCompat = newRegs
refOpts.Registries = newRegs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReferenceOptions.init() 基于 refConf.RegistryIDs 对已被 ClientOptions 过滤过一次的 map 再次过滤。若 WithRegistryIDs 指定的 ID 在第一次过滤后不存在,newRegs 为空,服务会静默走 URL 直连而不是注册中心,没有任何 error 或 warn,极难排查。

建议:newRegs 为空且原始 regs 非空时,应 return error 或至少打 warn 提示 registry ID 不存在。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Complete delete config package

4 participants