-
Notifications
You must be signed in to change notification settings - Fork 131
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
fix sync-ddl control #1130
fix sync-ddl control #1130
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
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. |
/run-all-tests |
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.
LGTM
drainer/syncer.go
Outdated
@@ -492,7 +492,7 @@ ForLoop: | |||
continue | |||
} | |||
} else if !ignore && s.cfg.DestDBType == "oracle" { | |||
if _, ok := stmt.(*ast.TruncateTableStmt); !ok { | |||
if _, ok := stmt.(*ast.TruncateTableStmt); !ok && s.cfg.SyncDDL { |
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'm a bit confuse why we only check s.cfg.SyncDDL
in such inner block. Is it bettern to check and skip all DDL event at L433. @lichunzhu
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.
Because we don't continue
here but just set shouldSkip = true
to make sure this table's schema can be updated in time. Maybe we should return an error in oracle dsyncer's loader, but I think return an error here is also okay.
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 mean we should move !s.cfg.SyncDDL
to L433 instead of check here.
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.
SGTM, but we need to sync this item at L433 too. @cartersz
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 97655ca
|
/run-all-tests |
What problem does this PR solve?
Issue Number: ref #1129
What is changed and how it works?
add SyncDDL switcher to the process that is whether drainer server should stop when non 'truncate table **' statement sync to drainer.
Check List
Tests
Code changes
Side effects
Related changes
tidb-ansible
repositoryRelease note