Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

syncer: table no need exist when drop #985

Merged
merged 5 commits into from
Sep 4, 2020

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Sep 3, 2020

What problem does this PR solve?

close #975

What is changed and how it works?

  • do not check table exist when drop table because we have added IF EXIST for DROP TABLE statement.

Tests

  • Integration test

@GMHDBJD GMHDBJD added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Sep 3, 2020
Copy link
Collaborator

@lance6716 lance6716 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

run_sql_source1 "drop table if exists \`all_mode\`.\`tb1\`;"
run_sql_source2 "drop table if exists \`all_mode\`.\`tb2\`;"
run_sql_source2 "drop table if exists \`all_mode\`.\`tb2\`;"
sleep 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about retry and check downstream table dropped, maybe quicker than 2s

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to check no error exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just want to replace sleep 2s this line 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check log in 5bb5f63

Copy link
Member

@csuzhangxc csuzhangxc 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

@@ -2100,8 +2100,6 @@ func (s *Syncer) trackDDL(usedSchema string, sql string, tableNames [][]*filter.
shouldSchemaExist = true
case *ast.DropTableStmt:
shouldExecDDLOnSchemaTracker = true
shouldSchemaExist = true
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to handle shouldSchemaExist for case *ast.DropDatabaseStmt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We create it if not exist below. But actually no need. Address in 56bf87f

@lance6716
Copy link
Collaborator

LGTM

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Sep 4, 2020
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@GMHDBJD GMHDBJD merged commit e92aced into pingcap:master Sep 4, 2020
@GMHDBJD GMHDBJD deleted the fixTableNotExists branch September 4, 2020 02:17
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #990

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Sep 4, 2020
@csuzhangxc csuzhangxc added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM status/PTAL This PR is ready for review. Add this label back after committing new changes labels Sep 4, 2020
@csuzhangxc csuzhangxc added this to the v2.0.0 RC.3 milestone Sep 4, 2020
csuzhangxc pushed a commit that referenced this pull request Sep 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to drop not exist table.
4 participants