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.

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

ddl: reduce the interval for checking "create table/schema" #7608

Merged
merged 7 commits into from Sep 12, 2018

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Sep 4, 2018

What problem does this PR solve?

After we put the DDL job into the job queue, the owner will handle the job in the background. We check every "tick" if the current job is completed. Handling the operation of "create table/schema" will be faster.

What is changed and how it works?

I changed the interval of "create table/schema" to 500ms.

Check List

ddl/ddl.go Outdated
@@ -447,6 +447,9 @@ func checkJobMaxInterval(job *model.Job) time.Duration {
if job.Type == model.ActionAddIndex {
return 3 * time.Second
}
if job.Type == model.ActionCreateTable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a TODO here.

Copy link
Member

Choose a reason for hiding this comment

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

Why not consider CreateDatabase/DropTable....?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shenli
"DropTable" has 4 states. So this operation takes longer than it takes.

Copy link
Contributor

@zhexuany zhexuany Sep 6, 2018

Choose a reason for hiding this comment

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

I am confused by the context. Anyone help me out?

request add a TODO.

TODO for what?

Why not consider CreateDatabase/DropTable....?

you mean we reduce tick of CreateDatabase/DropTable to 500ms?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they are the same story with CreateTable.

you mean we reduce tick of CreateDatabase/DropTable to 500ms?

Copy link
Member

Choose a reason for hiding this comment

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

@zimulala How long does it usually take to drop table/database?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shenli
This is the estimated time.

tidb> create database db;
Query OK, 0 rows affected (0.16 sec)
tidb> create table db.x(a int, b int);
Query OK, 0 rows affected (0.19 sec)
tidb> drop table db.x;
Query OK, 0 rows affected (0.39 sec)
tidb> drop database db;
Query OK, 0 rows affected (0.35 sec)

Copy link
Contributor

@winkyao winkyao Sep 6, 2018

Choose a reason for hiding this comment

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

@zhexuany We will use etcd to notify the job is done in the future, therefore we needn't wait an uncertain time here.

@zimulala zimulala changed the title ddl: reduce the interval for checking "create table" ddl: reduce the interval for checking "create table/schema" Sep 11, 2018
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 11, 2018
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 11, 2018
Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

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

LGTM

@ciscoxll ciscoxll added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Sep 11, 2018
@zz-jason zz-jason merged commit 794c4f4 into pingcap:master Sep 12, 2018
@zimulala zimulala deleted the create-table-interval branch November 8, 2018 08:13
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants