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: pre-split region for partitioned table #10221

Merged
merged 8 commits into from
Apr 30, 2019

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Reduce split region costs

What is changed and how it works?

Split region for partitioned table when creating

Check List

Tests

  • Unit test

@tiancaiamao
Copy link
Contributor Author

PTAL @crazycs520 @winkyao @zimulala

@codecov
Copy link

codecov bot commented Apr 22, 2019

Codecov Report

Merging #10221 into master will decrease coverage by 0.0102%.
The diff coverage is 92.8571%.

@@               Coverage Diff                @@
##             master     #10221        +/-   ##
================================================
- Coverage   77.6681%   77.6578%   -0.0103%     
================================================
  Files           411        411                
  Lines         85443      85444         +1     
================================================
- Hits          66362      66354         -8     
- Misses        14118      14128        +10     
+ Partials       4963       4962         -1

ddl/table.go Outdated Show resolved Hide resolved
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

ddl/table_split_test.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lonng lonng left a comment

Choose a reason for hiding this comment

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

LGTM

@lonng lonng added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 24, 2019
ddl/table.go Outdated
pi := tbInfo.GetPartitionInfo()
if pi != nil {
// Max partition count is 4096, should we sample and just choose some of the partition to split?
go func(pi *model.PartitionInfo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about not go? Since the split process can be really slow. Client could start to insert data before split complete.

Copy link
Contributor

@crazycs520 crazycs520 Apr 24, 2019

Choose a reason for hiding this comment

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

I add a session variable tidb_wait_table_split_finish to control use sync or async split. Maybe we can use it. in this PR: #10138
And because the split process can be really slow, I prefer to put the split process in ddl_api.go, not in the DDL owner to do this, otherwise sync split maybe block the DDL jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've move the code from table.go to ddl_api.go

@winkyao
Copy link
Contributor

winkyao commented Apr 24, 2019

@tiancaiamao Why we are necessary to split partition for partitioned tables? will it cause too many regions in tikv?

@tiancaiamao
Copy link
Contributor Author

PTAL @crazycs520 @winkyao

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM
Please fix the conflict.

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@jackysp jackysp merged commit 4cf3630 into pingcap:master Apr 30, 2019
@tiancaiamao tiancaiamao deleted the partition-table-split branch April 30, 2019 05:00
@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/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants