Repository navigation
br: complete the list of unrecoverable tables (#52531) - #69947
ti-chi-bot[bot] merged 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@Leavrth This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughSystem-table restore logic adds plan replayer table classification, expands unrecoverable-table filtering, and skips plan replayer tables during temporary-table replacement. ChangesSystem-table restore filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Poem Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
br/pkg/restore/snap_client/systable_restore.go (1)
315-344: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winResolve merge conflict markers and retain necessary functions.
Unresolved merge conflict markers are causing compilation failures. Since
isRenameableSysTableandremoveUserResourceGroupare still used elsewhere in the file, they must be preserved alongside the newly introduced plan replayer table helper.I have additionally renamed the new helper to
isPlanReplayerTable(singular) to remain consistent with existing conventions likeisUnrecoverableTable, and updated its map reference toplanReplayerTablesto fix the typo.🐛 Proposed fix to resolve merge conflicts
-<<<<<<< HEAD -func isRenameableSysTable(schemaName string, tableName string) bool { - tableMap, ok := renameableSysTables[schemaName] -======= -func isPlanReplayerTables(schemaName string, tableName string) bool { - tableMap, ok := planPeplayerTables[schemaName] ->>>>>>> 4b98d4e74c0 (br: complete the list of unrecoverable tables (`#52531`)) - if !ok { - return false - } - _, ok = tableMap[tableName] - return ok -} - -<<<<<<< HEAD -func removeUserResourceGroup(ctx context.Context, dbName string, execSQL func(context.Context, string) error) error { - sql := fmt.Sprintf("UPDATE %s SET User_attributes = JSON_REMOVE(User_attributes, '$.resource_group');", - utils.EncloseDBAndTable(dbName, sysUserTableName)) - if err := execSQL(ctx, sql); err != nil { - // FIXME: find a better way to check the error or we should check the version here instead. - if !strings.Contains(err.Error(), "Unknown column 'User_attributes' in 'field list'") { - return err - } - log.Warn("remove resource group meta failed, please ensure target cluster is newer than v6.6.0", logutil.ShortError(err)) - } - return nil -} - -======= ->>>>>>> 4b98d4e74c0 (br: complete the list of unrecoverable tables (`#52531`)) +func isRenameableSysTable(schemaName string, tableName string) bool { + tableMap, ok := renameableSysTables[schemaName] + if !ok { + return false + } + _, ok = tableMap[tableName] + return ok +} + +func isPlanReplayerTable(schemaName string, tableName string) bool { + tableMap, ok := planReplayerTables[schemaName] + if !ok { + return false + } + _, ok = tableMap[tableName] + return ok +} + +func removeUserResourceGroup(ctx context.Context, dbName string, execSQL func(context.Context, string) error) error { + sql := fmt.Sprintf("UPDATE %s SET User_attributes = JSON_REMOVE(User_attributes, '$.resource_group');", + utils.EncloseDBAndTable(dbName, sysUserTableName)) + if err := execSQL(ctx, sql); err != nil { + // FIXME: find a better way to check the error or we should check the version here instead. + if !strings.Contains(err.Error(), "Unknown column 'User_attributes' in 'field list'") { + return err + } + log.Warn("remove resource group meta failed, please ensure target cluster is newer than v6.6.0", logutil.ShortError(err)) + } + return nil +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/snap_client/systable_restore.go` around lines 315 - 344, Resolve the conflict markers in this section and retain all required functions: keep isRenameableSysTable and removeUserResourceGroup, while also adding the new helper as isPlanReplayerTable. Update that helper to reference the correctly named planReplayerTables map, preserving each function’s existing behavior and ensuring no merge markers remain.Source: Pipeline failures
🧹 Nitpick comments (2)
br/pkg/restore/snap_client/systable_restore.go (2)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in the variable name.
The variable name
planPeplayerTablescontains a typo. Consider renaming it toplanReplayerTablesfor clarity and correctness.♻️ Proposed refactor
-var planPeplayerTables = map[string]map[string]struct{}{ +var planReplayerTables = map[string]map[string]struct{}{ "mysql": { "plan_replayer_status": {}, "plan_replayer_task": {},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/snap_client/systable_restore.go` around lines 35 - 41, Rename the package-level variable planPeplayerTables to planReplayerTables and update every reference to it consistently, preserving the existing table mappings and behavior.
524-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the function call to match the renamed helper.
If you accept the renaming suggestion above, be sure to update the invocation here to use the singular
isPlanReplayerTable.♻️ Proposed refactor
- if isPlanReplayerTables(dbName, tableName) { + if isPlanReplayerTable(dbName, tableName) { return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/snap_client/systable_restore.go` around lines 524 - 527, Update the helper invocation in the restore flow to use the renamed singular symbol isPlanReplayerTable instead of isPlanReplayerTables, preserving the existing early return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@br/pkg/restore/snap_client/systable_restore.go`:
- Around line 315-344: Resolve the conflict markers in this section and retain
all required functions: keep isRenameableSysTable and removeUserResourceGroup,
while also adding the new helper as isPlanReplayerTable. Update that helper to
reference the correctly named planReplayerTables map, preserving each function’s
existing behavior and ensuring no merge markers remain.
---
Nitpick comments:
In `@br/pkg/restore/snap_client/systable_restore.go`:
- Around line 35-41: Rename the package-level variable planPeplayerTables to
planReplayerTables and update every reference to it consistently, preserving the
existing table mappings and behavior.
- Around line 524-527: Update the helper invocation in the restore flow to use
the renamed singular symbol isPlanReplayerTable instead of isPlanReplayerTables,
preserving the existing early return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c5114be5-ac12-4224-8bb2-90f30324103a
📒 Files selected for processing (1)
br/pkg/restore/snap_client/systable_restore.go
|
@YuJuncen need to fix the merge conflicts. |
Signed-off-by: Juncen Yu <yujuncen@pingcap.com>
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
🧹 Nitpick comments (1)
br/pkg/restore/snap_client/systable_restore.go (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the plan replayer map name.
planPeplayerTablescontains a typo. Rename it toplanReplayerTablesand update its lookup inisPlanReplayerTables. (raw.githubusercontent.com)As per coding guidelines, “Go code SHOULD be self-documenting through clear naming and structure.”
Proposed rename
-var planPeplayerTables = map[string]map[string]struct{}{ +var planReplayerTables = map[string]map[string]struct{}{ ... - tableMap, ok := planPeplayerTables[schemaName] + tableMap, ok := planReplayerTables[schemaName]Also applies to: 324-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@br/pkg/restore/snap_client/systable_restore.go` around lines 35 - 41, Rename the map variable planPeplayerTables to planReplayerTables and update every reference to it, including the lookup in isPlanReplayerTables; preserve the existing map contents and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@br/pkg/restore/snap_client/systable_restore.go`:
- Around line 35-41: Rename the map variable planPeplayerTables to
planReplayerTables and update every reference to it, including the lookup in
isPlanReplayerTables; preserve the existing map contents and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a39d9ee-0b6f-4f38-908e-a6b317bc5ac4
📒 Files selected for processing (1)
br/pkg/restore/snap_client/systable_restore.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69947 +/- ##
================================================
Coverage ? 56.2363%
================================================
Files ? 1852
Lines ? 673319
Branches ? 0
================================================
Hits ? 378650
Misses ? 266720
Partials ? 27949
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
|
@YuJuncen: Your lgtm message is repeated, so it is ignored. 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: Leavrth, YuJuncen 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
ti-chi-bot
Bot
added
cherry-pick-approved
|
/retest |
4 participants
This is an automated cherry-pick of #52531
What problem does this PR solve?
Issue Number: close #52530
Problem Summary:
we need to complete the list of unrecoverable tables.
What changed and how does it work?
complete the list of unrecoverable tables.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit