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

variable, ddl: add session variable 'tidb_ddl_reorg_priority' to set operation priority of ddl reorg #7116

Merged
merged 7 commits into from
Aug 8, 2018
Merged

variable, ddl: add session variable 'tidb_ddl_reorg_priority' to set operation priority of ddl reorg #7116

merged 7 commits into from
Aug 8, 2018

Conversation

winkyao
Copy link
Contributor

@winkyao winkyao commented Jul 20, 2018

What have you changed? (mandatory)

Add session variable 'tidb_ddl_reorg_priority' to set operation priority of ddl reorg. Before this PR, the operation in adding indices is definitely in low priority, In some case, we want to finish DDL job as fast as possible, after this PR, we can set the variable tidb_ddl_reorg_priority to priority_high to achieve it.

What is the type of the changes? (mandatory)

  • New feature (non-breaking change which adds functionality)

How has this PR been tested? (mandatory)

UT

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

YES

Does this PR affect tidb-ansible update? (mandatory)

NO

Does this PR need to be added to the release notes? (mandatory)

YES:

add session variable 'tidb_ddl_reorg_priority' to set operation priority of DDL reorg.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@shenli
Copy link
Member

shenli commented Jul 20, 2018

Please add proper labels.

@shenli
Copy link
Member

shenli commented Jul 20, 2018

This PR should be metioned in the release notes.

@shenli
Copy link
Member

shenli commented Jul 20, 2018

/run-all-tests

@winkyao winkyao added component/DDL-need-LGT3 release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 23, 2018
@winkyao
Copy link
Contributor Author

winkyao commented Jul 24, 2018

@ciscoxll @zimulala @crazycs520 PTAL

@ciscoxll
Copy link
Contributor

@winkyao Please resolve conflicts.

@winkyao
Copy link
Contributor Author

winkyao commented Aug 3, 2018

@ciscoxll @zimulala @crazycs520 PTAL


cli.mu.Lock()
cli.mu.checkFlags = checkRequestOff
cli.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't understand the test is used to test what?

Copy link
Contributor Author

@winkyao winkyao Aug 6, 2018

Choose a reason for hiding this comment

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

test if the following set priority operation takes effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which line to check if the following set priority operation takes effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can look into checkRequestClient.SendRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it~


cli.mu.Lock()
cli.mu.checkFlags = checkRequestOff
cli.mu.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it~

meta/meta.go Outdated
job := &model.Job{}
job := &model.Job{
// for compability, if the job is enqueued by old version TiDB and Priority field is omitted,
// set the default priority to kv.PriorityLow
Copy link
Contributor

Choose a reason for hiding this comment

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

end with .

meta/meta.go Outdated
@@ -504,8 +504,16 @@ func (m *Meta) getDDLJob(key []byte, index int64) (*model.Job, error) {
return nil, errors.Trace(err)
}

job := &model.Job{}
job := &model.Job{
// for compability, if the job is enqueued by old version TiDB and Priority field is omitted,
Copy link
Contributor

Choose a reason for hiding this comment

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

s/for/For

meta/meta.go Outdated
err = job.Decode(value)
// check job.Priority is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/check/Check

crazycs520
crazycs520 previously approved these changes Aug 7, 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

case "priority_normal":
s.DDLReorgPriority = kv.PriorityNormal
case "priority_high":
s.DDLReorgPriority = kv.PriorityHigh
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a default clause?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do agree with @zhexuany.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

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.

reset LGTM.

@ciscoxll
Copy link
Contributor

ciscoxll commented Aug 8, 2018

LGTM

@ciscoxll ciscoxll added the status/LGT3 The PR has already had 3 LGTM. label Aug 8, 2018
@winkyao winkyao merged commit 359df6e into pingcap:master Aug 8, 2018
@winkyao winkyao deleted the add_variable_control_reorg_priority branch August 8, 2018 09:16
@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
release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

7 participants