external store: use separate ctx for GCS clients #60402
Conversation
Signed-off-by: lance6716 <[email protected]>
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
ti-chi-bot
bot
added
the
size/M
|
Hi @lance6716. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
br/pkg/storage/gcs.go:370
- Potential nil pointer dereference: ensure that s.clientCancel is non-nil before calling it in Close() to prevent runtime panic.
s.clientCancel()
br/pkg/storage/gcs.go:462
- [nitpick] Consider refactoring the custom cancellation logic in Reset(). Instead of using context.Background() with a goroutine and a done channel to propagate cancellation, use context.WithCancel(ctx) to automatically inherit cancellation from the provided context.
go func() {
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #60402 +/- ##
================================================
+ Coverage 73.1230% 74.8519% +1.7289%
================================================
Files 1714 1760 +46
Lines 474566 482742 +8176
================================================
+ Hits 347017 361342 +14325
+ Misses 106225 98641 -7584
- Partials 21324 22759 +1435
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
br/pkg/storage/gcs.go
Outdated
| clientCtx, clientCancel := context.WithCancel(context.Background()) | ||
| done := make(chan struct{}) | ||
| defer close(done) | ||
| go func() { | ||
| select { | ||
| case <-done: | ||
| case <-ctx.Done(): | ||
| clientCancel() | ||
| } | ||
| }() | ||
|
|
||
| s.clients = make([]*storage.Client, gcsClientCnt) | ||
| eg, egCtx := util.NewErrorGroupWithRecoverWithCtx(ctx) | ||
| firstErr := atomic.NewError(nil) | ||
| wg := util.WaitGroupWrapper{} | ||
| for i := range s.clients { | ||
| eg.Go(func() error { | ||
| client, err := storage.NewClient(egCtx, s.clientOps...) | ||
| wg.RunWithLog(func() { | ||
| client, err := storage.NewClient(clientCtx, s.clientOps...) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| firstErr.CompareAndSwap(nil, err) | ||
| clientCancel() | ||
| return | ||
| } | ||
| client.SetRetry(storage.WithErrorFunc(shouldRetry), storage.WithPolicy(storage.RetryAlways)) | ||
| s.clients[i] = client | ||
| return nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
maybe this one
s.clients = make([]*storage.Client, 0, gcsClientCnt)
wg := util.WaitGroupWrapper{}
cliCh := make(chan *storage.Client)
wg.RunWithLog(func() {
for range gcsClientCnt {
select {
case cli := <-cliCh:
s.clients = append(s.clients, cli)
case <-ctx.Done():
clientCancel()
return
case <-clientCtx.Done():
return
}
}
})
firstErr := atomic.NewError(nil)
for range gcsClientCnt {
wg.RunWithLog(func() {
client, err := storage.NewClient(clientCtx, s.clientOps...)
if err != nil {
firstErr.CompareAndSwap(nil, err)
clientCancel()
return
}
client.SetRetry(storage.WithErrorFunc(shouldRetry), storage.WithPolicy(storage.RetryAlways))
select {
case cliCh <- client:
case <-clientCtx.Done():
}
})
}There was a problem hiding this comment.
@D3Hunter I didn't get your point. Do you want to avoid the ugly code? ⬇️
done := make(chan struct{})
defer close(done)
go func() {
select {
case <-done:
case <-ctx.Done():
clientCancel()
}
}()
There was a problem hiding this comment.
yes, the done channel is not needed, and I want the routine managed by waitGroup
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, Leavrth The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/hold feel free to unhold |
ti-chi-bot
bot
added
the
do-not-merge/hold
Signed-off-by: lance6716 <[email protected]>
|
/unhold |
ti-chi-bot
bot
added
size/L
|
/retest |
|
@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
Benjamin2037
added
the
needs-cherry-pick-release-8.5
|
/retest |
|
/retest |
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
bot
removed
the
needs-cherry-pick-release-8.5
Benjamin2037
added
the
needs-cherry-pick-release-8.5
Signed-off-by: ti-chi-bot <[email protected]>
This comment was marked as duplicate.
This comment was marked as duplicate.
Benjamin2037
added
the
needs-cherry-pick-release-8.1
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #60155
Problem Summary:
What changed and how does it work?
see original issue for more information
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.