Skip to content
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.

Sign up for GitHub

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

Jump to bottom

brie: fix the problem that ddl restored by BR via SQL is not replicated to downstream #22862

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

brie: fix the problem that ddl restored by BR via SQL is not replicated to downstream #22862

wants to merge 7 commits into from
+60 −2

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Feb 22, 2021 •
edited
Loading

What problem does this PR solve?

Problem Summary:

The cluster topology: TiDB -> TiCDC/TiDB-Binlog -> MySQL.

When I use BR via SQL to restore tables to the upstream TiDB, the downstream MySQL hadn't received the correct create table DDL job. This problem is similar to pingcap/br#406.

What is changed and how it works?

What's Changed: Add show create database/table function and set the SQL to session.

How it Works:

The following CreateTableWithInfo/CreateSchemaWithInfo will get the create SQL from the session.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    Test use the hotfix TiDB to do restore job with TiCDC/TiDB-Binlog. The downstream MySQL can create tables successfully.

Release note

  • BR via SQL: fix the problem that ddl restored by BR via SQL is not replicated to downstream

@lichunzhu lichunzhu requested a review from a team as a code owner February 22, 2021 03:28
@lichunzhu lichunzhu requested review from qw4990 and removed request for a team February 22, 2021 03:28
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Feb 22, 2021
@lichunzhu
Copy link
Contributor Author

/cc 3pointer,kennytm

@github-actions github-actions bot added the sig/execution SIG execution label Feb 22, 2021
@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Feb 22, 2021
@kennytm
Copy link
Contributor

kennytm commented Feb 22, 2021

/lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2021
@lichunzhu
Copy link
Contributor Author

/unlabel sig/infra

@lichunzhu
Copy link
Contributor Author

/label sig/migrate

@ti-chi-bot
Copy link
Member

@lichunzhu: Those labels are not set on the issue: sig/infra

In response to this:

/remove-sig infra

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 ti-community-infra/tichi repository.

@lichunzhu
Copy link
Contributor Author

/remove-sig sql-infra

@ti-chi-bot ti-chi-bot removed the sig/sql-infra SIG: SQL Infra label Feb 23, 2021
@ti-chi-bot
Copy link
Member

@lichunzhu: Those labels are not set on the issue: sig/sql-infra

In response to this:

/remove-sig sql-infra

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 ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@lichunzhu: Those labels are not set on the issue: sig/infra

In response to this:

/remove-sig infra

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 ti-community-infra/tichi repository.

@lichunzhu
Copy link
Contributor Author

/remove-sig execution

@ti-chi-bot ti-chi-bot removed the sig/execution SIG execution label Feb 23, 2021
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Feb 23, 2021
@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 23, 2021
@lance6716
Copy link
Contributor

lance6716 commented Feb 25, 2021 •
edited
Loading

For implementation of CreateDatabase, how about we re-use implementation of Execute to execute a SQL

@lichunzhu
Copy link
Contributor Author

For implementation of CreateDatabase, how about we re-use implementation of Execute to execute a SQL

I prefer not to re-use Execute. ddl has existing interfaces to create schemas/tables through DBInfo/TableInfo. Executing the transferred SQL will increase the risk of meeting an error.

@lichunzhu
Copy link
Contributor Author

/cc overvenus

@overvenus overvenus requested a review from zier-one March 2, 2021 12:28
lichunzhu
lichunzhu commented Mar 2, 2021
View reviewed changes
executor/brie.go Outdated
const (
defaultCapOfCreateTable = 512
defaultCapOfCreateDatabase = 64
brViaSQLComment = `/*from(br_via_sql)*/`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we distinguish br via sql with br https://github.com/pingcap/br/blob/master/pkg/gluetidb/glue.go#L30?
Or we can set this to /*from(br)*/ since we will merge br into tidb in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*from(br)*/ +1

overvenus
overvenus reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2021
@ti-chi-bot
Copy link
Member

@lichunzhu: PR needs rebase.

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.

@qw4990 qw4990 removed their request for review June 21, 2021 03:34
@ti-chi-bot ti-chi-bot bot deleted a comment from ti-chi-bot Dec 19, 2023
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 19, 2023

@lichunzhu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-br-integration-test f1c54ba link true /test pull-br-integration-test
pull-integration-ddl-test f1c54ba link true /test pull-integration-ddl-test

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@overvenus overvenus overvenus left review comments

@3pointer 3pointer Awaiting requested review from 3pointer

@kennytm kennytm Awaiting requested review from kennytm

@zier-one zier-one Awaiting requested review from zier-one

Assignees
No one assigned
Labels
first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/execution SIG execution sig/migrate sig/sql-infra SIG: SQL Infra size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lichunzhu @kennytm @ti-chi-bot @lance6716 @overvenus @ti-srebot