feat(ticdc): prevent using the same TiDB cluster as both upstream and downstream #3956
Conversation
ti-chi-bot
bot
added
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 |
Summary of ChangesHello @wlwilliamx, 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 enhances TiCDC's robustness by implementing a critical safeguard against self-replication. It ensures that a TiDB cluster cannot serve as both the data source and the replication target for a changefeed, thereby preventing potential data inconsistencies and operational issues. The change integrates a new cluster identification and comparison logic into the changefeed lifecycle management, providing immediate feedback and preventing misconfigurations. 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
|
ti-chi-bot
bot
added
size/XL
![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 valuable feature to prevent a changefeed from using the same TiDB cluster as both upstream and downstream, which could lead to replication loops. The implementation correctly adds checks at changefeed creation, update, and resume. The core logic for comparing cluster IDs is well-encapsulated in the new pkg/check package and is accompanied by good unit and integration tests.
My main feedback is regarding code duplication in api/v2/changefeed.go, where the check logic is repeated in three different API handlers. Refactoring this into a shared helper function would improve maintainability. I've also included a couple of minor suggestions for improving code clarity and error handling in pkg/check/cluster.go.
Overall, this is a solid contribution that enhances the robustness of TiCDC.
| // Check whether the upstream and downstream are the different cluster. | ||
| notSame, err := check.UpstreamDownstreamNotSame(ctx, pdClient, cfConfig) | ||
| if err != nil { | ||
| _ = c.Error(err) | ||
| return | ||
| } | ||
| if !notSame { | ||
| _ = c.Error(errors.ErrSameUpstreamDownstream.GenWithStack( | ||
| "TiCDC does not support creating a changefeed with the same TiDB cluster " + | ||
| "as both the source and the target for the changefeed.")) | ||
| return | ||
| } |
There was a problem hiding this comment.
The logic to check if upstream and downstream clusters are the same is duplicated across CreateChangefeed, ResumeChangefeed (lines 694-705), and UpdateChangefeed (lines 888-899). This makes the code harder to maintain.
To improve this, you could extract the duplicated logic into a helper method on *OpenAPIV2. This method can be parameterized to handle the slightly different error messages for each context (creating, resuming, updating).
Here's an example of what the helper method could look like:
func (h *OpenAPIV2) verifyUpstreamDownstreamNotSame(
c *gin.Context,
pdClient pd.Client,
cfConfig *config.ChangefeedConfig,
action string,
) bool {
ctx := c.Request.Context()
notSame, err := check.UpstreamDownstreamNotSame(ctx, pdClient, cfConfig)
if err != nil {
_ = c.Error(err)
return false
}
if !notSame {
errMsg := fmt.Sprintf(
"TiCDC does not support %s a changefeed with the same TiDB cluster "+
"as both the source and the target for the changefeed.", action)
_ = c.Error(errors.ErrSameUpstreamDownstream.GenWithStack(errMsg))
return false
}
return true
}Then, you can replace the duplicated blocks with a single call to this helper, for instance in CreateChangefeed:
if !h.verifyUpstreamDownstreamNotSame(c, pdClient, cfConfig, "creating") {
return
}This refactoring will make the code more DRY and easier to manage in the future.
| if upPD == nil { | ||
| return false, cerrors.New("pd client is nil") | ||
| } | ||
| if changefeedCfg == nil { | ||
| return false, cerrors.New("changefeed config is nil") | ||
| } |
There was a problem hiding this comment.
For better error handling and consistency, consider using a structured error from the cerrors package instead of cerrors.New(). cerrors.ErrInternalCheckFailed seems appropriate for these nil checks.
For example:
if upPD == nil {
return false, cerrors.ErrInternalCheckFailed.GenWithStack("pd client is nil")
}
if changefeedCfg == nil {
return false, cerrors.ErrInternalCheckFailed.GenWithStack("changefeed config is nil")
}This provides more context to the error and aligns with the error handling patterns used elsewhere in the codebase.
| if upPD == nil { | |
| return false, cerrors.New("pd client is nil") | |
| } | |
| if changefeedCfg == nil { | |
| return false, cerrors.New("changefeed config is nil") | |
| } | |
| if upPD == nil { | |
| return false, cerrors.ErrInternalCheckFailed.GenWithStack("pd client is nil") | |
| } | |
| if changefeedCfg == nil { | |
| return false, cerrors.ErrInternalCheckFailed.GenWithStack("changefeed config is nil") | |
| } |
pkg/check/cluster.go
Outdated
|
|
||
| _, db, err := newMySQLConfigAndDBFn(ctx, changefeedCfg.ChangefeedID, uri, changefeedCfg) | ||
| if err != nil { | ||
| return 0, true, cerrors.Trace(err) |
There was a problem hiding this comment.
When newMySQLConfigAndDBFn returns an error, you are returning isTiDB as true. This is slightly counter-intuitive, as a connection failure means we cannot determine if the downstream is a TiDB cluster.
While the calling function UpstreamDownstreamNotSame correctly handles the error and ignores the boolean value, returning false for isTiDB in this error case would make the function's contract clearer and less surprising to future readers.
| return 0, true, cerrors.Trace(err) | |
| return 0, false, cerrors.Trace(err) |
ti-chi-bot
bot
added
size/XXL
ti-chi-bot
bot
removed
do-not-merge/work-in-progress
|
CC @tenfyzhong |
|
CC @asddongmen |
| // verify sinkURI | ||
| cfConfig := info.ToChangefeedConfig() | ||
| // Check whether the upstream and downstream are the different cluster. | ||
| notSame, err := check.UpstreamDownstreamNotSame(ctx, pdClient, cfConfig) |
There was a problem hiding this comment.
Confusing Naming and Logic: The function name UpstreamDownstreamNotSame with double negative logic (!notSame) makes the code harder to understand.
Consider renaming to use positive semantics for better readability:
// Rename to IsSameCluster
isSame, err := check.IsSameCluster(ctx, pdClient, cfConfig)
if err != nil {
_ = c.Error(err)
return
}
if isSame {
_ = c.Error(errors.ErrSameUpstreamDownstream.GenWithStack(...))
return
}Either naming would be clearer than the current notSame variable combined with !notSame check. The current double negative makes the logic harder to follow.
| if !hasRow { | ||
| keyspace = value | ||
| hasRow = true | ||
| continue |
There was a problem hiding this comment.
Optimization: You can directly break here instead of continue. Within the same TiDB cluster, all instances should report the same keyspace-name value.
Since we're just checking consistency across rows, we could simplify this to:
if !hasRow {
keyspace = value
hasRow = true
break // We have the first value, no need to continue
}| if err := rows.Err(); err != nil { | ||
| return "", cerrors.Trace(err) | ||
| } | ||
| if !hasRow { |
There was a problem hiding this comment.
Classic Architecture Compatibility: When no keyspace-name is found, we should return common.DefaultKeyspaceName instead of returning an error. This ensures compatibility with TiDB Classic architecture.
In TiDB Classic (non-Next-Gen), the SHOW CONFIG command may not return the keyspace-name row at all, or the configuration may not exist. Returning an error here would break the check for Classic deployments.
Suggested fix:
if !hasRow {
// TiDB Classic doesn't expose keyspace-name, use default
return common.DefaultKeyspaceNamme, nil
}2 participants
What problem does this PR solve?
TiCDC does not support using the same TiDB logical cluster as both the upstream and downstream, because it can easily lead to self-replication loops or unexpected data behaviors.
In TiDB Next-Gen, a physical cluster can contain multiple logical clusters (keyspaces). Different keyspaces should be treated as different clusters from TiCDC's perspective. So this PR keeps rejecting same upstream/downstream for the same keyspace, while allowing cross-keyspace replication within the same physical cluster.
Issue Number: close #3949
What is changed and how it works?
mysql.tidb(cluster_id).cluster_idare the same, query the downstream keyspace via:show config where type = 'tidb' and name = 'keyspace-name'(cluster_id, keyspace)as the cluster identity, so cross-keyspace is allowed but same keyspace is rejected.same_upstream_downstream.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
N/A
Release note