api(ticdc): add ineligibleTables/eligible tables info when creating/updating changefeed#12500
api(ticdc): add ineligibleTables/eligible tables info when creating/updating changefeed#12500wk989898 wants to merge 4 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
[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 |
ti-chi-bot
bot
added
the
size/XL
Summary of ChangesHello @wk989898, 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 significantly improves the transparency and user experience for managing TiCDC changefeeds by exposing more detailed table information through both a new API endpoint and enhanced CLI commands. It allows users to easily inspect which tables are considered, eligible, or ineligible for replication, and provides verbose output options for in-depth analysis during changefeed creation, resumption, and updates. This addresses previous limitations regarding visibility into table filtering and eligibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 introduces a new API endpoint /api/v2/get_all_tables and updates the CLI to display more detailed table information when creating or updating a changefeed. The changes are generally well-implemented. However, I've identified a few areas for improvement regarding code duplication, error handling in the new API handler, and user-facing output in the CLI commands. Addressing these points will enhance the maintainability and user experience of the new features.
| var kvStore tidbkv.Storage | ||
| // if PDAddrs is empty, use the default upstream | ||
| if len(cfg.PDAddrs) == 0 { | ||
| up, err := getCaptureDefaultUpstream(h.capture) | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return | ||
| } | ||
| kvStore = up.KVStorage | ||
| } else { | ||
| credential := cfg.PDConfig.toCredential() | ||
| var err error | ||
| kvStore, err = h.helpers.createTiStore(ctx, cfg.PDAddrs, credential) | ||
| if err != nil { | ||
| _ = c.Error(errors.Trace(err)) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for acquiring kvStore is duplicated in the verifyTable function. To improve maintainability and reduce redundancy, consider extracting this logic into a private helper method on OpenAPIV2.
| f, err := filter.NewFilter(replicaCfg, "") | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return | ||
| } | ||
| _, ineligibleTables, eligibleTables, allTables, err := entry. | ||
| VerifyTables(f, kvStore, cfg.StartTs) | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return |
There was a problem hiding this comment.
The errors from filter.NewFilter and entry.VerifyTables are passed to c.Error without being wrapped. This can lead to a loss of context, making debugging more difficult. It's recommended to wrap these errors, for example, using cerror.WrapError(cerror.ErrInternalServerError, err), to provide more information about where the error originated.
| f, err := filter.NewFilter(replicaCfg, "") | |
| if err != nil { | |
| _ = c.Error(err) | |
| return | |
| } | |
| _, ineligibleTables, eligibleTables, allTables, err := entry. | |
| VerifyTables(f, kvStore, cfg.StartTs) | |
| if err != nil { | |
| _ = c.Error(err) | |
| return | |
| f, err := filter.NewFilter(replicaCfg, "") | |
| if err != nil { | |
| _ = c.Error(cerror.WrapError(cerror.ErrInternalServerError, err)) | |
| return | |
| } | |
| _, ineligibleTables, eligibleTables, allTables, err := entry. | |
| VerifyTables(f, kvStore, cfg.StartTs) | |
| if err != nil { | |
| _ = c.Error(cerror.WrapError(cerror.ErrInternalServerError, err)) | |
| return | |
| } |
| cmd.Printf("[WARN] Force to replicate some ineligible tables, "+ | ||
| "these tables do not have a primary key or a not-null unique key: %#v\n"+ | ||
| "[WARN] This may cause data redundancy, "+ | ||
| "please refer to the official documentation for details.\n", | ||
| tables.IneligibleTables) | ||
| } else { | ||
| cmd.Printf("[WARN] Some tables are not eligible to replicate, "+ | ||
| "because they do not have a primary key or a not-null unique key: %#v\n", | ||
| tables.IneligibleTables) |
There was a problem hiding this comment.
The warning messages for ineligible tables use %#v to print the table list. This format is not user-friendly as it prints the Go struct representation. Please use the formatTableNames helper function for a cleaner, more readable output, similar to how it's used for the --verbose output.
if cf.Config.ForceReplicate {
cmd.Printf("[WARN] Force to replicate some ineligible tables, "+
"these tables do not have a primary key or a not-null unique key: %s\n"+
"[WARN] This may cause data redundancy, "+
"please refer to the official documentation for details.\n",
formatTableNames(tables.IneligibleTables))
} else {
cmd.Printf("[WARN] Some tables are not eligible to replicate, "+
"because they do not have a primary key or a not-null unique key: %s\n",
formatTableNames(tables.IneligibleTables))| cmd.Printf("IneligibleTables: %s\n", formatTableNames(tables.IneligibleTables)) | ||
| cmd.Printf("AllTables: %s\n", formatTableNames(tables.AllTables)) | ||
| } | ||
| return err |
There was a problem hiding this comment.
Returning err here is slightly confusing because at this point, err is guaranteed to be nil due to the preceding error check. For better clarity, it's recommended to explicitly return nil.
return nil| cmd.Printf("[WARN] Force to replicate some ineligible tables, "+ | ||
| "these tables do not have a primary key or a not-null unique key: %#v\n"+ | ||
| "[WARN] This may cause data redundancy, "+ | ||
| "please refer to the official documentation for details.\n", | ||
| tables.IneligibleTables) | ||
| } else { | ||
| cmd.Printf("[WARN] Some tables are not eligible to replicate, "+ | ||
| "because they do not have a primary key or a not-null unique key: %#v\n", | ||
| tables.IneligibleTables) |
There was a problem hiding this comment.
The warning messages for ineligible tables use %#v to print the table list. This format is not user-friendly as it prints the Go struct representation. Please use the formatTableNames helper function for a cleaner, more readable output, similar to how it's used for the --verbose output.
| cmd.Printf("[WARN] Force to replicate some ineligible tables, "+ | |
| "these tables do not have a primary key or a not-null unique key: %#v\n"+ | |
| "[WARN] This may cause data redundancy, "+ | |
| "please refer to the official documentation for details.\n", | |
| tables.IneligibleTables) | |
| } else { | |
| cmd.Printf("[WARN] Some tables are not eligible to replicate, "+ | |
| "because they do not have a primary key or a not-null unique key: %#v\n", | |
| tables.IneligibleTables) | |
| if newInfo.Config.ForceReplicate { | |
| cmd.Printf("[WARN] Force to replicate some ineligible tables, "+ | |
| "these tables do not have a primary key or a not-null unique key: %s\n"+ | |
| "[WARN] This may cause data redundancy, "+ | |
| "please refer to the official documentation for details.\n", | |
| formatTableNames(tables.IneligibleTables)) | |
| } else { | |
| cmd.Printf("[WARN] Some tables are not eligible to replicate, "+ | |
| "because they do not have a primary key or a not-null unique key: %s\n", | |
| formatTableNames(tables.IneligibleTables)) |
|
@wk989898: 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. |
1 participant
What problem does this PR solve?
Issue Number: close #12501
What is changed and how it works?
It introduces a new API endpoint /api/v2/get_all_tables and updates the corresponding CLI commands to display this information, including a new --verbose flag. However, I've found a critical bug related to variable shadowing in the resume command, an error handling issue in a new API handler, and several instances of code duplication and misleading CLI output that should be addressed.
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