lightning: add store write limiter #35193

sleepymole · 2022-06-06T14:21:54Z

What problem does this PR solve?

Issue Number: close #35192

Problem Summary:

We want to use lightning to import data to an online cluster. To reduce the impact on online services, lightning needs to limit the write rate to the cluster.

What is changed and how it works?

Add a new config tikv-importer.store-write-bwlimit and limit the write bytes to TiKV nodes.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

lightning: add store write limiter

ti-chi-bot · 2022-06-06T14:21:55Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

D3Hunter
lichunzhu

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2022-06-06T14:37:43Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/6797a6d27e5894fee0cf4334405f9a090cb307df

br/pkg/lightning/backend/local/local.go

br/tidb-lightning.toml

dsdashun · 2022-06-13T07:59:50Z

br/pkg/lightning/backend/local/local.go

+	flushKVs := func() error {
+		for i := range clients {
+			if local.writeLimiter != nil {
+				if err := local.writeLimiter.WaitN(ctx, storeIDs[i], int(size)); err != nil {


If I understand correctly, the rate limiter controls the writing limit for each TiKV storage nodes. Since the burst is just 20% more of the limit, if the sending size is larger than the and the limit / burst size, I'm afraid the actual write rate is smaller than the setting value ?

For example, suppose rate limiter is 1000, burst is 1200. If the sending size is, say 3000, and there are 3 nodes. For the first storage's rate limiter, it takes around 3 seconds to prepare the 3000 B storage data, and the rate is 1000B/s , which is OK. However, when iterating on the second storage node, it takes another 3 seconds to prepare the second storage data, and at that time the data has not sent actually, and so does the third storage node. At that time where preparation finishes, 3+3+3 = 9 seconds has elapsed. But for each node, only 3000 bytes are sent. So the actual writing rate for each node is the sending size / total prepare time, which is 3000 / 9.

Maybe those several rate limiter could be waiting in parallel ?

Your analysis is correct. There is a flushLimit, which is not greater than the limit. So the WaitN size is usually less than the burst.

In my test, the size is about 3M. In practice, there is no need to set such a small limit. Because to achieve this speed, we can use tidb-backend instead.

At that time where preparation finishes, 3+3+3 = 9 seconds has elapsed. But for each node, only 3000 bytes are sent. So the actual writing rate for each node is the sending size / total prepare time, which is 3000 / 9.

I thought of it again. I think this is as expected. Because kvs are sent to tikv node one by one for each region. The total send size is 9000, we need 9 seconds to send these data. It's unnecessary to wait for the rate limiter in parallel unless we actually send data to tikv in parallel.

D3Hunter

rest lgtm

D3Hunter · 2022-06-15T11:12:04Z

br/pkg/lightning/backend/local/localhelper.go

+		burst = limit + limit/5
+	} else {
+		// If overflowed, set burst to math.MaxInt.
+		burst = math.MaxInt


maybe adjust them during config.adjust, and add a log

This is an internal implementation. burst is not configurable.

br/pkg/lightning/backend/local/local.go

D3Hunter · 2022-06-15T11:40:48Z

br/pkg/lightning/backend/local/local.go

+	// the speed of write more smooth.
+	flushLimit := int64(math.MaxInt64)
+	if local.writeLimiter != nil {
+		flushLimit = int64(local.writeLimiter.limit / 10)


why use 1/10?

Simply set a value that is smaller than the limit value. The call interval of the limiter will not exceed 100ms.

D3Hunter · 2022-06-15T11:47:40Z

br/pkg/lightning/config/config.go

@@ -532,6 +532,7 @@ type TikvImporter struct {

 	EngineMemCacheSize      ByteSize `toml:"engine-mem-cache-size" json:"engine-mem-cache-size"`
 	LocalWriterMemCacheSize ByteSize `toml:"local-writer-mem-cache-size" json:"local-writer-mem-cache-size"`
+	StoreWriteBWLimit       ByteSize `toml:"store-write-bwlimit" json:"store-write-bwlimit"`


what's bw? batch write?

maybe max-store-write-rate?

@sunzhaoyang ptal too

bw means bandwitch. It is similar to rysnc --bwlimit.

br/pkg/lightning/backend/local/local.go

sleepymole · 2022-06-16T08:17:14Z

/run-integration-br-test

dsdashun

rest LGTM

br/tidb-lightning.toml

dsdashun

LGTM

ti-chi-bot · 2022-06-20T11:48:25Z

@dsdashun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

sleepymole · 2022-06-20T11:49:00Z

/run-integration-br-test

lichunzhu

LGTM

sleepymole · 2022-06-21T02:51:54Z

/merge

ti-chi-bot · 2022-06-21T02:51:57Z

This pull request has been accepted and is ready to merge.

Commit hash: 8ec642b

sleepymole · 2022-06-21T03:33:37Z

/run-unit-test

hawkingrei · 2022-06-21T03:42:54Z

/run-mysql-test

sre-bot · 2022-06-21T05:35:13Z

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 1, success 10, total 11	22 min	Existing failure
idc-jenkins-ci-tidb/common-test	✅ all 12 tests passed	8 min 46 sec	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	26 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 16 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	6 min 3 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	5 min 36 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 7 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 10 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 42 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

* upstream/master: sessionctx: support encoding and decoding session variables (pingcap#35531) planner: add batch_point_get access object (pingcap#35230) sessionctx: set skipInit false for TiDBOptProjectionPushDown and TiDBOptAggPushDown (pingcap#35491) *: add support for disabling noop variables (pingcap#35496) lightning: add store write limiter (pingcap#35193) expression: avoid padding 0 when implicitly cast to binary (pingcap#35053) types: fix creating partition tables fail in ANSI_QUOTES mode (pingcap#35379) executor: add the missed runtime stats when the index merge partial task returns 0 row (pingcap#35297) statistics: batch insert topn and bucket when saving table stats (pingcap#35326) parser: Add support for INTERVAL expr unit + expr (pingcap#30253) (pingcap#35390) config: add missing nodelay example (pingcap#35255) *: Introduce `OptimisticTxnContextProvider` for optimistic txn (pingcap#35131) statistics: fix panic when using wrong globalStats.Indices key (pingcap#35424) *: fix store token is up to the limit in test (pingcap#35374) *: enable more flaky and update bazel config (pingcap#35500) ddl: expose getPrimaryKey() as a public method of model.TableInfo (pingcap#35512) expression, util: add `KeyWithoutTrimRightSpace` for collator (pingcap#35475) infoschema: try on each PD member until one succeeds or all fail (pingcap#35285)

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2022

sleepymole added the component/lightning This issue is related to Lightning of TiDB. label Jun 6, 2022

sleepymole force-pushed the online-import-2 branch from 7d9740c to 23baa4c Compare June 6, 2022 14:27

lightning: add store write limiter

31c89e5

sleepymole force-pushed the online-import-2 branch from 23baa4c to 31c89e5 Compare June 6, 2022 17:57

sleepymole requested review from sunzhaoyang, lichunzhu and D3Hunter June 7, 2022 03:13

sleepymole and others added 3 commits June 7, 2022 15:48

rename option name

9defbc8

Merge branch 'master' into online-import-2

47d0bb5

Merge branch 'master' into online-import-2

6a89123

dsdashun reviewed Jun 13, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2022

D3Hunter reviewed Jun 15, 2022

View reviewed changes

sleepymole added 2 commits June 16, 2022 16:13

address comments

aafe78b

Merge remote-tracking branch 'origin/master' into online-import-2

4802250

D3Hunter approved these changes Jun 16, 2022

View reviewed changes

ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 16, 2022

sleepymole requested a review from dsdashun June 20, 2022 01:52

Merge branch 'master' into online-import-2

bca655b

dsdashun reviewed Jun 20, 2022

View reviewed changes

br/tidb-lightning.toml Outdated Show resolved Hide resolved

remove duplicated line

8ec642b

dsdashun approved these changes Jun 20, 2022

View reviewed changes

lichunzhu approved these changes Jun 21, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 21, 2022

Merge branch 'master' into online-import-2

6636cdc

Merge branch 'master' into online-import-2

6797a6d

ti-chi-bot merged commit 2648989 into pingcap:master Jun 21, 2022

sleepymole deleted the online-import-2 branch June 21, 2022 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: add store write limiter #35193

lightning: add store write limiter #35193

sleepymole commented Jun 6, 2022 •

edited

Loading

ti-chi-bot commented Jun 6, 2022 •

edited

Loading

sre-bot commented Jun 6, 2022 •

edited

Loading

dsdashun Jun 13, 2022

sleepymole Jun 13, 2022

sleepymole Jun 13, 2022

sleepymole Jun 15, 2022 •

edited

Loading

D3Hunter left a comment

D3Hunter Jun 15, 2022

sleepymole Jun 16, 2022

D3Hunter Jun 15, 2022

sleepymole Jun 16, 2022

D3Hunter Jun 15, 2022

D3Hunter Jun 15, 2022

sleepymole Jun 16, 2022

sleepymole commented Jun 16, 2022

dsdashun left a comment

dsdashun left a comment

ti-chi-bot commented Jun 20, 2022

sleepymole commented Jun 20, 2022

lichunzhu left a comment

sleepymole commented Jun 21, 2022

ti-chi-bot commented Jun 21, 2022

sleepymole commented Jun 21, 2022

hawkingrei commented Jun 21, 2022

sre-bot commented Jun 21, 2022

lightning: add store write limiter #35193

lightning: add store write limiter #35193

Conversation

sleepymole commented Jun 6, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jun 6, 2022 • edited Loading

sre-bot commented Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepymole Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepymole commented Jun 16, 2022

dsdashun left a comment

Choose a reason for hiding this comment

dsdashun left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Jun 20, 2022

sleepymole commented Jun 20, 2022

lichunzhu left a comment

Choose a reason for hiding this comment

sleepymole commented Jun 21, 2022

ti-chi-bot commented Jun 21, 2022

sleepymole commented Jun 21, 2022

hawkingrei commented Jun 21, 2022

sre-bot commented Jun 21, 2022

TiDB MergeCI notify

sleepymole commented Jun 6, 2022 •

edited

Loading

ti-chi-bot commented Jun 6, 2022 •

edited

Loading

sre-bot commented Jun 6, 2022 •

edited

Loading

sleepymole Jun 15, 2022 •

edited

Loading