-
Notifications
You must be signed in to change notification settings - Fork 287
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
syncer(dm): fix different output format for operate-schema get (#5824) #5884
syncer(dm): fix different output format for operate-schema get (#5824) #5884
Conversation
Signed-off-by: ti-chi-bot <[email protected]>
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
…ti-chi-bot/ticdc into cherry-pick-5824-to-release-5.4
/run-verify |
case pb.SchemaOp_GetSchema: | ||
// when task is paused, schemaTracker is closed. We get the table structure from checkpoint. | ||
ti := s.checkpoint.GetTableInfo(req.Database, req.Table) | ||
if ti == nil { | ||
s.tctx.L().Info("table schema is not in checkpoint, fetch from downstream", | ||
zap.String("table", sourceTable.String())) | ||
targetTable := s.route(sourceTable) | ||
return dbconn.GetTableCreateSQL(s.tctx.WithContext(ctx), s.downstreamTrackConn, targetTable.String()) |
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.
I think we don't need to remove the dbconn.GetTableCreateSQL() calling and replace it with SHOW CREATE TABLE
calling. But rather:
result, err2 := dbconn.GetTableCreateSQL(s.tctx.WithContext(ctx), s.downstreamTrackConn, targetTable.String())
result = strings.Replace(result, fmt.Sprintf("CREATE TABLE %s", quotes.QuoteName(targetTable.Name)), fmt.Sprintf("CREATE TABLE %s", quotes.QuoteName(sourceTable.Name)), 1)
return utils.CreateTableSQLToOneRow(result), err2
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.
dbconn.GetTableCreateSQL() is not included in the old-version branch. Moreover, the committed logic is not equal to dbconn.GetTableCreateSQL() since conn.QuerySQL() between the versions are of different APIs.
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.
OK. Can we reuse (*schema.Tracker).GetCreateTable()
? In my opinion, this PR mainly changes the table name from target table name to source table name, and changes the sql into a single line ( not in this version ). Maybe :
targetTable := s.route(sourceTable)
result, err2 := s.schemaTracker.GetCreateTable(ctx, targetTable)
result = strings.Replace(result, fmt.Sprintf("CREATE TABLE %s", quotes.QuoteName(targetTable.Name)), fmt.Sprintf("CREATE TABLE %s", quotes.QuoteName(sourceTable.Name)), 1)
return result, err2
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.
Actually I am not sure about this. In my opinion, getting the table from schema tracker is an incorrect version. Getting schema can only be operated when task is paused. However, when the task is paused, the schema tracker is down and nothing would return. I have tried to build the original version and observed this phenomenon.
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.
OK. Seems PR #5350 fixed this problem. Does that PR also need to be cherry-picked into 5.4 ? @lance6716
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.
done by #5415
/close |
@ForwardStar: Closed this PR. In response to this:
Instructions 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/test-infra repository. |
This is an automated cherry-pick of #5824
What problem does this PR solve?
Issue Number: close #5688
What is changed and how it works?
If the checkpoint doesn't exist and the schema is retrieved from the downstream, replace the downstream table name with the requested upstream table name
Delete all
\n
in the SQL results for the operate-schema resultCheck List
Tests
Questions
Will it cause performance regression or break compatibility? No.
Do you need to update user documentation, design documentation or monitoring documentation? No.
Release note