ddl: Fix ddl owner doesn't use etcd keyspace prefix #60403
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @djshow832. 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. |
|
/ok-to-test |
|
@ystaticy: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #60403 +/- ##
================================================
+ Coverage 73.1167% 75.6218% +2.5051%
================================================
Files 1711 1760 +49
Lines 473138 492121 +18983
================================================
+ Hits 345943 372151 +26208
+ Misses 105937 97450 -8487
- Partials 21258 22520 +1262
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/cc @D3Hunter |
pkg/ddl/owner_mgr.go
Outdated
| @@ -68,6 +71,10 @@ func (om *ownerManager) Start(ctx context.Context, store kv.Storage) error { | |||
| if cli == nil { | |||
| return errors.New("etcd client is nil, maybe the server is not started with PD") | |||
| } | |||
| // nil in unit tests | |||
| if store != nil && !reflect.ValueOf(store).IsNil() { | |||
There was a problem hiding this comment.
if store is nil, cli will be nil, code won't reach here
There was a problem hiding this comment.
No, cli is not nil. TestOwnerManager fails because of it.
There was a problem hiding this comment.
I think it is not reasonable to pass nil kv.Storage. How about fixing TestOwnerManager by passing a real store?
ti-chi-bot
bot
added
size/M
|
/retest |
|
/retest |
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, tangenta, ystaticy 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
|
/retest |
8 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
bb7133
added
the
needs-cherry-pick-release-8.5
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 #60401
Problem Summary:
Previously, the etcdCli used for DDL owner was created by the domain with a keyspace prefix, so that each keyspace has a separate DDL owner.
However, in #57179, the global DDL owner creates etcdCli by itself without the keyspace prefix, so all the keyspaces share the same DDL owner. When bootstrapping, the DDL owner is occupied by another TiDB, and this TiDB keeps in the loop of the bootstrap.
What changed and how does it work?
Set the etcdCli prefix before creating the DDL owner.
Check List
Tests
testtestSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.