Update dependencies and restore etcd API#3245
Update dependencies and restore etcd API#3245utafrali wants to merge 1 commit intoapache:developfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3245 +/- ##
===========================================
+ Coverage 47.91% 47.97% +0.05%
===========================================
Files 467 467
Lines 34045 34045
===========================================
+ Hits 16314 16334 +20
+ Misses 16407 16388 -19
+ Partials 1324 1323 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates etcdv3 client construction to use the newer gost etcd client API that accepts an options struct + context, and refreshes several Go module dependencies accordingly.
Changes:
- Switch etcdv3 client creation from
gxetcd.NewClient(...)togxetcd.NewClientWithOptions(ctx, options). - Add
contextusage for etcdv3 client initialization. - Bump dependency versions in
go.modand updatego.sumentries.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| remoting/etcdv3/client.go | Migrates etcdv3 client initialization to the new NewClientWithOptions API and introduces context passing. |
| go.mod | Updates versions for gost, logrus, and several golang.org/x/* modules. |
| go.sum | Adds corresponding checksums for the updated module versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if container.Client() == nil { | ||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) | ||
| if err != nil { |
| // Client lose connection with etcd server | ||
| if container.Client().GetRawClient() == nil { | ||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) | ||
| if err != nil { |
|
|
||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) | ||
| if err != nil { |
| // new Client | ||
| if container.Client() == nil { | ||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) |
There was a problem hiding this comment.
All three NewClientWithOptions call sites use context.Background(), which creates an uncancellable context. ValidateClient is called under container.ClientLock(), so if etcd is unreachable and the dial blocks, the lock is held indefinitely — freezing the entire application.
NewClientWithOptions internally calls context.WithCancel(ctx), so cancellation from the caller side is fully supported but unused here.
Suggestion: At minimum, use context.WithTimeout(context.Background(), options.Timeout) to bound the blocking time:
ctx, cancel := context.WithTimeout(context.Background(), options.Timeout)
defer cancel()
newClient, err := gxetcd.NewClientWithOptions(ctx, options)Ideally, accept a context.Context parameter in ValidateClient and NewServiceDiscoveryClient to give callers full control over cancellation.
|
client, err := gxetcd.NewClient(gxetcd.MetadataETCDV3Client, addresses, timeout, 1)This inconsistency means metadata report will not benefit from TLS/auth support, and if a future gost version removes Suggestion: Update to client, err := gxetcd.NewClientWithOptions(context.Background(), &gxetcd.Options{
Name: gxetcd.MetadataETCDV3Client,
Endpoints: addresses,
Timeout: timeout,
Heartbeat: 1,
}) |
|
The PR checklist states "I confirm the target branch is develop", but this PR actually targets |
I have changed the target branch to develop. |
|
@CAICAIIs check this |
|
Done, pushed the fix. |
|
actually, what you should do is to modify https://github.com/dubbogo/gost instead of dubbo-go repo. What you should do is to do extally the same as dubbogo/gost#131 |
AlexStocks
left a comment
AlexStocks
left a comment
There was a problem hiding this comment.
PR 主要是更新 gost 依赖（v1.14.3 → v1.14.4）并恢复 etcd client API。
发现一个问题：gxetcd.NewClientWithOptions(context.Background(), options) 使用了硬编码的 context.Background()。建议改成支持调用者传入 context，或者至少使用 context.TODO() 表明这是临时的。
其他方面看起来没问题：依赖更新是常规操作，gost 的 API 变更（从 NewClient 到 NewClientWithOptions）应该是向下兼容的。
| // new Client | ||
| if container.Client() == nil { | ||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) |
There was a problem hiding this comment.
这里硬编码了 context.Background()。建议使用 context.TODO() 表明这是临时的，或者考虑添加一个 context 参数让调用者控制。
| // Client lose connection with etcd server | ||
| if container.Client().GetRawClient() == nil { | ||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) |
There was a problem hiding this comment.
同上，硬编码 context.Background()。
| } | ||
|
|
||
| newClient, err := gxetcd.NewClient(options.Name, options.Endpoints, options.Timeout, options.Heartbeat) | ||
| newClient, err := gxetcd.NewClientWithOptions(context.Background(), options) |
There was a problem hiding this comment.
同上，硬编码 context.Background()。
|
Please resolve the conflict when you have time~ |
utafrali
force-pushed
the
fix/issue-3243-update-restore-the-previously-removed-ap
branch
from
39cc231 to
c7025d9
Compare
|
Description
Fixes #3243
Bumped the dependencies to newer versions. The main thing is restoring the etcd API in the client that was previously removed - it's needed for grpc compatibility.
Checklist
develop