-
Notifications
You must be signed in to change notification settings - Fork 298
mounter(ticdc): fix truncate table cause mounter failed (#12431) #12447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-8.1
Are you sure you want to change the base?
mounter(ticdc): fix truncate table cause mounter failed (#12431) #12447
Conversation
|
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. |
Summary of ChangesHello @ti-chi-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a bug where truncating a partitioned table caused the mounter to fail. The fix involves correctly identifying and marking the old physical partition IDs as truncated. The overall logic is sound and the test changes align with the fix. I have one suggestion to improve the robustness of the implementation and prevent a potential panic.
| newPartitionIDMap := make(map[int64]struct{}, len(newPi.NewPartitionIDs)) | ||
| for _, partition := range newPi.Definitions { | ||
| newPartitionIDMap[partition.ID] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block has a potential nil pointer dereference if newPi is nil. Although TRUNCATE TABLE on a partitioned table should result in a new partitioned table, it's safer to handle the case where newPi could be nil for robustness.
Additionally, the map is sized using len(newPi.NewPartitionIDs), but it's populated from newPi.Definitions in the following loop. It would be more correct and robust to size it with len(newPi.Definitions).
newPartitionIDMap := make(map[int64]struct{})
if newPi != nil {
newPartitionIDMap = make(map[int64]struct{}, len(newPi.Definitions))
for _, partition := range newPi.Definitions {
newPartitionIDMap[partition.ID] = struct{}{}
}
}|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## release-8.1 #12447 +/- ##
================================================
Coverage ? 57.2632%
================================================
Files ? 854
Lines ? 126706
Branches ? 0
================================================
Hits ? 72556
Misses ? 48696
Partials ? 5454 🚀 New features to boost your workflow:
|
|
@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. |
This is an automated cherry-pick of #12431
What problem does this PR solve?
Issue Number: close #12430
What is changed and how it works?
If the table is a partition table, we have to record the partition table id when executing the truncate table ddl.
Check List
Tests
Before this PR:
After this PR:
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note