consumer: ignore stale event after renaming table in storage consumer#4871
consumer: ignore stale event after renaming table in storage consumer#4871ti-chi-bot[bot] merged 3 commits intopingcap:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
size/M
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the storage consumer to correctly handle table rename operations by updating DDL watermarks for both the old and new table identities. It introduces helper functions to parse DDL statements using the TiDB parser. Review feedback highlights the need to remove an unused regular expression and its corresponding import, and suggests adding safety checks to the type assertions and slice accesses during DDL parsing to avoid potential panics.
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "regexp" |
There was a problem hiding this comment.
The regexp package is imported but only used for the renameTableQueryRe variable, which is itself unused. Since the implementation uses the TiDB parser instead, this import should be removed.
| fakePartitionNumForSchemaFile = -1 | ||
| ) | ||
|
|
||
| var renameTableQueryRe = regexp.MustCompile(`(?is)^rename\s+table\s+(.+?)\s+to\s+(.+?)$`) |
There was a problem hiding this comment.
The renameTableQueryRe variable is unused. The logic in getRenameTableOldTableKey correctly uses the TiDB parser, which is a more reliable way to handle SQL statements. This variable should be removed.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
ti-chi-bot
Bot
added
needs-1-more-lgtm
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, flowbehappy, wlwilliamx 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 |
4 participants
What problem does this PR solve?
Issue Number: close #4334
What is changed and how it works?
Decode rename ddl query and update the old table name in tableDDLWatermark
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note