ddl: scatter the regions of table when creating them #10980

jackysp · 2019-06-28T08:11:09Z

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

It’s a bit late to schedule the region when PD found the hotspot. It is better to scatter them when they are empty.

What is changed and how it works?

Scatter the regions of the tables when creating them.

Check List

Tests

Manual test (add detailed scripts or steps below)
Created 100 tables and compare the regions distribution.
master:

This PR:

Code changes

Has exported variable/fields change

Side effects

Increased code complexity

codecov · 2019-06-28T08:34:10Z

Codecov Report

Merging #10980 into master will decrease coverage by 0.139%.
The diff coverage is 48.75%.

@@               Coverage Diff                @@
##             master     #10980        +/-   ##
================================================
- Coverage   81.1735%   81.0344%   -0.1391%     
================================================
  Files           421        422         +1     
  Lines         89916      89689       -227     
================================================
- Hits          72988      72679       -309     
- Misses        11687      11749        +62     
- Partials       5241       5261        +20

jackysp · 2019-06-30T10:22:43Z

/run-integration-common-test

jackysp · 2019-06-30T12:34:05Z

/run-all-tests

jackysp · 2019-06-30T14:02:11Z

/run-integration-common-test tidb-test=pr/844

jackysp · 2019-06-30T14:59:54Z

For 100 tables, waiting to scatter finish costs 24 min, and no waiting cost 6 min. Try to make the default value of tidb_wait_split_region_finish to off.

jackysp · 2019-07-01T03:12:11Z

Change tidb_wait_split_region_finish to global scope. It is a global only variable, so it keeps the same behavior as the global only variables in MySQL.

$ mysql -uroot test
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Welcome to the MySQL monitor.  Commands end with ; or \g.
Your MySQL connection id is 15
Server version: 8.0.16 Homebrew

Copyright (c) 2000, 2019, Oracle and/or its affiliates. All rights reserved.

Oracle is a registered trademark of Oracle Corporation and/or its
affiliates. Other names may be trademarks of their respective
owners.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

mysql> load data local infile "~/tmp.csv" into table t;
Query OK, 1 row affected (0.07 sec)
Records: 1  Deleted: 0  Skipped: 0  Warnings: 0

mysql> set global local_infile=0;
Query OK, 0 rows affected (0.00 sec)

mysql> load data local infile "~/tmp.csv" into table t;
ERROR 1148 (42000): The used command is not allowed with this MySQL version

-- set local_infile to on in another session.
mysql> load data local infile "~/tmp.csv" into table t;
Query OK, 1 row affected (0.05 sec)
Records: 1  Deleted: 0  Skipped: 0  Warnings: 0

jackysp · 2019-07-01T03:13:56Z

/run-all-tests

coocood · 2019-07-01T07:54:28Z

store/tikv/split_region.go

-}
-
-func (s *tikvStore) splitRegion(splitKey kv.Key) (*metapb.Region, error) {
+func (s *tikvStore) SplitRegion(splitKey kv.Key, scatter bool) (uint64, error) {


Need comment for the returned uint64 value.

I think SplitRegion is better just used for splitting region, scatter region is another action. Use a boolean to indicate different actions is not clean code. Keep SplitRegionAndScatter is better?

SplitRegionAndScatter is even bad. I think it’s worse to combine the names of the actions together as the function name. And we also need an interface named SplitRegion. The best way is to add a standalone interface for scattering, but now we have at least one interface saved, and split and scatter are really strongly related. If we need to use scatter separately in the future, I think we can separate them again.

crazycs520 · 2019-07-01T09:44:57Z

sessionctx/variable/sysvar.go

@@ -697,7 +697,7 @@ var defaultSysVars = []*SysVar{
 	{ScopeGlobal | ScopeSession, TiDBOptJoinReorderThreshold, strconv.Itoa(DefTiDBOptJoinReorderThreshold)},
 	{ScopeSession, TiDBCheckMb4ValueInUTF8, BoolToIntStr(config.GetGlobalConfig().CheckMb4ValueInUTF8)},
 	{ScopeSession, TiDBSlowQueryFile, ""},
-	{ScopeSession, TiDBWaitSplitRegionFinish, BoolToIntStr(DefTiDBWaitSplitRegionFinish)},
+	{ScopeGlobal, TiDBWaitSplitRegionFinish, BoolToIntStr(DefTiDBWaitSplitRegionFinish)},


Why change to Global scope?

jackysp · 2019-07-01T13:26:41Z

/run-all-tests

jackysp · 2019-07-02T03:04:24Z

PTAL @crazycs520 @winkyao @coocood

coocood · 2019-07-02T03:20:07Z

LGTM

crazycs520 · 2019-07-02T00:25:17Z

ddl/split_region.go

+	} else {
+		regionIDs = append(regionIDs, splitRecordRegion(store, tbInfo.ID, scatter))
+	}
+	regionIDs = append(regionIDs, splitIndexRegion(store, tbInfo, scatter)...)


Split index region for a common table, Is it expected?

It used to only split a table region for a common table, doesn't split the index regions.

emm... Yes, it is expected.

crazycs520

LGTM

crazycs520 · 2019-07-03T13:19:57Z

@winkyao PTAL

jackysp · 2019-07-09T09:48:31Z

PTAL @winkyao

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

ddl/ddl_api.go

winkyao · 2019-07-10T02:09:34Z

ddl/ddl_api.go

 			}
-		} else if atomic.LoadUint32(&EnableSplitTableRegion) != 0 {
+			scatterRegion := variable.TiDBOptOn(val)


if the variable.GetGlobalSystemVar return err, scatterRegion should be false?

Yes, I think so.

winkyao

Rest LGTM

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

winkyao

LGTM

winkyao · 2019-07-10T03:01:28Z

/run-all-tests

crazycs520 · 2019-07-10T09:00:04Z

@jackysp Please cherry-pick this PR to v3.0 and v2.1.

Conflicts: ddl/ddl_api.go ddl/table.go sessionctx/variable/tidb_vars.go sessionctx/variable/varsutil.go store/tikv/split_region.go

jackysp added the component/DDL-need-LGT3 label Jun 28, 2019

jackysp added the status/WIP label Jun 28, 2019

jackysp force-pushed the scatter_table branch from b23ca83 to d79b236 Compare June 28, 2019 14:16

jackysp removed the status/WIP label Jun 29, 2019

jackysp requested review from crazycs520, tiancaiamao and coocood June 29, 2019 14:57

jackysp force-pushed the scatter_table branch from d79b236 to 3f48412 Compare June 29, 2019 16:08

jackysp changed the title ~~ddl: scatter the regions of table and index when creating them~~ ddl: scatter the regions of table when creating them Jun 29, 2019

jackysp force-pushed the scatter_table branch 2 times, most recently from 3f48412 to 8f75385 Compare June 30, 2019 03:41

jackysp force-pushed the scatter_table branch from 9b9032d to 8cfaeef Compare June 30, 2019 10:31

jackysp added the status/all tests passed label Jul 1, 2019

coocood reviewed Jul 1, 2019

View reviewed changes

crazycs520 reviewed Jul 1, 2019

View reviewed changes

crazycs520 reviewed Jul 2, 2019

View reviewed changes

ddl: scatter regions of table and index when creating them

d6b9d5a

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

jackysp force-pushed the scatter_table branch from cf494e3 to d6b9d5a Compare July 9, 2019 11:48

winkyao reviewed Jul 10, 2019

View reviewed changes

ddl/ddl_api.go Outdated Show resolved Hide resolved

winkyao reviewed Jul 10, 2019

View reviewed changes

address comments

afc230e

Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>

winkyao approved these changes Jul 10, 2019

View reviewed changes

Merge branch 'master' into scatter_table

1f3eff6

winkyao added require-LGT3 Indicates that the PR requires three LGTM. status/LGT3 The PR has already had 3 LGTM. and removed require-LGT3 Indicates that the PR requires three LGTM. labels Jul 10, 2019

jackysp merged commit 89baed8 into pingcap:master Jul 10, 2019

jackysp added a commit to jackysp/tidb that referenced this pull request Jul 11, 2019

ddl: scatter the regions of table when creating them (pingcap#10980)

5a94129

Conflicts: ddl/ddl_api.go ddl/table.go sessionctx/variable/tidb_vars.go sessionctx/variable/varsutil.go store/tikv/split_region.go

jackysp mentioned this pull request Jul 11, 2019

ddl: scatter the regions of table when creating them (#10980) #11213

Merged

jackysp added a commit to jackysp/tidb that referenced this pull request Jul 11, 2019

ddl: scatter the regions of table when creating them (pingcap#10980)

ff4c5a6

Conflicts: ddl/ddl_api.go ddl/table.go sessionctx/variable/tidb_vars.go sessionctx/variable/varsutil.go store/tikv/split_region.go

jackysp mentioned this pull request Jul 11, 2019

ddl: scatter the regions of table when creating them (#10980) #11216

Merged

zimulala pushed a commit that referenced this pull request Jul 12, 2019

ddl: scatter the regions of table when creating them (#10980) (#11216)

0ab7a2b

zimulala pushed a commit that referenced this pull request Jul 12, 2019

ddl: scatter the regions of table when creating them (#10980) (#11213)

65a1531

jackysp deleted the scatter_table branch February 27, 2020 13:32

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl: scatter the regions of table when creating them #10980

ddl: scatter the regions of table when creating them #10980

jackysp commented Jun 28, 2019 •

edited

Loading

codecov bot commented Jun 28, 2019 •

edited

Loading

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019 •

edited

Loading

jackysp commented Jul 1, 2019 •

edited

Loading

jackysp commented Jul 1, 2019

coocood Jul 1, 2019

winkyao Jul 1, 2019

jackysp Jul 1, 2019

crazycs520 Jul 1, 2019

jackysp commented Jul 1, 2019

jackysp commented Jul 2, 2019

coocood commented Jul 2, 2019

crazycs520 Jul 2, 2019

jackysp Jul 2, 2019

crazycs520 left a comment

crazycs520 commented Jul 3, 2019

jackysp commented Jul 9, 2019

winkyao Jul 10, 2019 •

edited

Loading

jackysp Jul 10, 2019

winkyao left a comment

winkyao left a comment

winkyao commented Jul 10, 2019

crazycs520 commented Jul 10, 2019

ddl: scatter the regions of table when creating them #10980

ddl: scatter the regions of table when creating them #10980

Conversation

jackysp commented Jun 28, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Jun 28, 2019 • edited Loading

Codecov Report

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019

jackysp commented Jun 30, 2019 • edited Loading

jackysp commented Jul 1, 2019 • edited Loading

jackysp commented Jul 1, 2019

coocood Jul 1, 2019

Choose a reason for hiding this comment

winkyao Jul 1, 2019

Choose a reason for hiding this comment

jackysp Jul 1, 2019

Choose a reason for hiding this comment

crazycs520 Jul 1, 2019

Choose a reason for hiding this comment

jackysp commented Jul 1, 2019

jackysp commented Jul 2, 2019

coocood commented Jul 2, 2019

crazycs520 Jul 2, 2019

Choose a reason for hiding this comment

jackysp Jul 2, 2019

Choose a reason for hiding this comment

crazycs520 left a comment

Choose a reason for hiding this comment

crazycs520 commented Jul 3, 2019

jackysp commented Jul 9, 2019

winkyao Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

jackysp Jul 10, 2019

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

winkyao left a comment

Choose a reason for hiding this comment

winkyao commented Jul 10, 2019

crazycs520 commented Jul 10, 2019

jackysp commented Jun 28, 2019 •

edited

Loading

codecov bot commented Jun 28, 2019 •

edited

Loading

jackysp commented Jun 30, 2019 •

edited

Loading

jackysp commented Jul 1, 2019 •

edited

Loading

winkyao Jul 10, 2019 •

edited

Loading