br: load necessary db infos for backup and restore#64982
br: load necessary db infos for backup and restore#64982ti-chi-bot[bot] merged 22 commits intopingcap:masterfrom
Conversation
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
…nter/63278 Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
ti-chi-bot
bot
added
release-note
|
Hi @YuJuncen. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
ti-chi-bot
bot
added
size/XXL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #64982 +/- ##
================================================
+ Coverage 70.7690% 77.8492% +7.0801%
================================================
Files 1902 2001 +99
Lines 519022 562563 +43541
================================================
+ Hits 367307 437951 +70644
+ Misses 127178 121568 -5610
+ Partials 24537 3044 -21493
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
|
@YuJuncen: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn 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. |
| } | ||
|
|
||
| // IsBRRelatedDB checks whether dbOriginName is a temporary database created by BR. | ||
| func IsBRRelatedDB(dbOriginName string) bool { |
There was a problem hiding this comment.
this can be moved to BR pkg
There was a problem hiding this comment.
Some of test cases sill relies on it. Moving it to br may cause cycle dependency. (Also it is hard to move those test cases to BR package...)
br/pkg/gluetidb/infoschema_filter.go
Outdated
| if !isAllowed { | ||
| return false | ||
| } |
There was a problem hiding this comment.
does this want to impl "if any table ID is allowed, then we shouldn't skip"? if so, we shouldn't negate isAllowed
There was a problem hiding this comment.
It seems I forgot these codes. I have rewritten them.
…_skip_full_load Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
P0 Issue FoundBR can block cluster DDL (MDL-enabled) by skipping per-job schema-version updates
Files Affected by This PR:
|
|
/retest |
|
@YuJuncen: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn 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. |
pkg/infoschema/issyncer/filter.go
Outdated
| @@ -0,0 +1,37 @@ | |||
| // Copyright 2025 PingCAP, Inc. | |||
br/pkg/gluetidb/infoschema_filter.go
Outdated
| func (f *brInfoSchemaFilter) SkipLoadDiff(diff *model.SchemaDiff, latestIS infoschema.InfoSchema) (skip bool) { | ||
| defer func() { | ||
| if skip { | ||
| log.Warn("skip load a schema diff due to configuration.", zap.Any("diff", diff), zap.Int64("version", diff.Version)) |
There was a problem hiding this comment.
log.Warn && zap.Any("diff", diff) may cause noice？
Consider use INFO or just log diff.Type/SchemaID/TableID/OldSchemaID
| return &brInfoSchemaFilter{allow: allow} | ||
| } | ||
|
|
||
| func (f *brInfoSchemaFilter) SkipLoadDiff(diff *model.SchemaDiff, latestIS infoschema.InfoSchema) (skip bool) { |
There was a problem hiding this comment.
SkipLoadDiff treats SchemaDiff.SchemaID as a database ID and does latestIS.SchemaByID(SchemaID) , but for several DDLs SchemaID is not a DB ID, so BR may incorrectly skip their diffs and keep stale global metadata:
- Placement policy diffs: SchemaID is the policy ID for create/alter/drop. BR only special-cases ActionCreatePlacementPolicy, not alter/drop.
- Resource group diffs: SchemaID is the resource group ID. BR has no special-cases, so create/alter/drop can be skipped.
- Impact: BR’s embedded domain/infoSchema can become inconsistent for these objects during diff-load, causing incorrect behavior if BR code relies on up-to-date policy/resource-group info.
|
|
||
| func (dm *domainMap) getWithEtcdClient(store kv.Storage, etcdClient *clientv3.Client) (d *domain.Domain, err error) { | ||
| func (dm *domainMap) GetOrCreateWithFilter(store kv.Storage, filter issyncer.Filter) (d *domain.Domain, err error) { | ||
| return dm.getWithEtcdClient(store, nil, filter) |
There was a problem hiding this comment.
Domain reuse ignores filter changes: the domain cache key is only store.UUID(), so GetOrCreateDomainWithFilter only applies the filter when the domain is first created. If a domain already exists (with a different/no filter), subsequent calls won’t recreate or update it.
Is it expected?
There was a problem hiding this comment.
Yes, as its name OrCreateWithFilter. Perhaps filter ID can be added to domain map's key but for now it seems that isn't pretty useful. I will add a comment to clearify this.
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
@YuJuncen: PRs from untrusted users cannot be marked as trusted with DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3pointer, D3Hunter, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@YuJuncen: PRs from untrusted users cannot be marked as trusted with DetailsIn 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 |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
@YuJuncen: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
BornChanger
added
the
needs-cherry-pick-release-8.5
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #64833
Problem Summary:
See the issue.
What changed and how does it work?
Added a filter to
loader, which allows the caller to load a subset of tables only.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.