prepare_snap: establish connection to all stores before pausing admin #51449
Conversation
Signed-off-by: Yu Juncen <[email protected]>
ti-chi-bot
bot
added
release-note-none
|
Hi @YuJuncen. 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/test-infra repository. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #51449 +/- ##
================================================
+ Coverage 70.6487% 72.3499% +1.7011%
================================================
Files 1462 1475 +13
Lines 435031 454834 +19803
================================================
+ Hits 307344 329072 +21728
+ Misses 108388 105728 -2660
- Partials 19299 20034 +735
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| clients[store.Id] = cli | ||
| } | ||
|
|
||
| for id, cli := range clients { |
There was a problem hiding this comment.
why use two loops? seems it can be done without clients map
There was a problem hiding this comment.
The first createAndCacheStream must happens after ALL connections established. I think we cannot remove this. Or would you give a code snippet?
There was a problem hiding this comment.
for _, store := range stores {
cli, err := p.env.ConnectToStore(ctx, store.Id)
if err != nil {
return errors.Annotatef(err, "failed to dial the store %d", store.Id)
}
if err := p.createAndCacheStream(ctx, cli, store.Id); err != nil {
return errors.Annotatef(err, "failed to create and cache stream for store %d", id)
}
}
There was a problem hiding this comment.
THis will make createAndCacheStream concurrency with ConnectToStore, recall that:
The first createAndCacheStream must happens after ALL connections established.
There was a problem hiding this comment.
...then what's problem with that?
|
/unit-test |
|
/test unit-test |
|
@YuJuncen: 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/test-infra repository. |
Signed-off-by: Yu Juncen <[email protected]>
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
/retest |
|
@BornChanger: 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/test-infra repository. |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
@BornChanger: 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/test-infra repository. |
|
/retest |
|
@BornChanger: 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/test-infra repository. |
|
/retest |
|
@BornChanger: 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/test-infra repository. |
|
/ok-to-test |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3pointer, BornChanger 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 |
|
@YuJuncen: The following test failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
|
/label needs-cherry-pick-release-6.5 |
ti-chi-bot
bot
added
the
needs-cherry-pick-release-6.5
|
In response to a cherrypick label: new pull request created to branch |
|
/retest-required |
BornChanger
added
needs-cherry-pick-release-7.1
Signed-off-by: ti-chi-bot <[email protected]>
|
In response to a cherrypick label: new pull request created to branch |
|
In response to a cherrypick label: new pull request created to branch |
…pingcap#51449) (pingcap#51) (pingcap#63) close pingcap#51448 Co-authored-by: 山岚 <[email protected]>
…pingcap#51449) (pingcap#51) close pingcap#51448 Co-authored-by: 山岚 <[email protected]>
…pingcap#51449) (pingcap#51730) (pingcap#86) close pingcap#51448 Co-authored-by: Ti Chi Robot <[email protected]>
What problem does this PR solve?
Issue Number: close #51448
Problem Summary:
Now if we cannot connect to a TiKV, it will block at creating connection.
But, in this scenario, some of TiKVs' admin commands are paused. That will impact the TP workload more.
What changed and how does it work?
After this PR, we won't pause the admin commands before connections to all TiKV nodes established.
Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.