br: complete the list of unrecoverable tables (#52531)#69947
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
|
@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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughSystem-table restore logic adds plan replayer table classification, expands unrecoverable-table filtering, and skips plan replayer tables during temporary-table replacement. The helper section contains unresolved merge-conflict markers. ChangesSystem-table restore filtering
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
|
@ti-chi-bot: 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. |
2 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