br: optimize communication with tikv stores. #31722 by MoCuishle28 · Pull Request #35147 · pingcap/tidb

MoCuishle28 · 2022-06-05T14:55:36Z

Signed-off-by: Gaoming Zhang zhanggaoming028@gmail.com

What problem does this PR solve?

Optimize communication with tikv stores.
Add ResetSpeedLimit function to reset the speed-limit of TiKV after the restore phase is done.

Issue Number: close #31722

Problem Summary:
Need to optimize communication with tikv stores.

What is changed and how it works?

Use the workPool field in the restore.Client to implement concurrent communication with TiKV stores.
Remove the rateLimit field of FileImporter.
Add ResetSpeedLimit function to reset the speed-limit of TiKV after the restore phase is done.

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

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

None

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

ti-chi-bot · 2022-06-05T14:55:37Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

3pointer
fengou1

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.

Details

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

sre-bot · 2022-06-05T15:05:34Z

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

MoCuishle28 · 2022-06-05T15:10:26Z

Question:

Do We really need to reset the speed-limit of TiKV stores?
How should the error returned by the ResetSpeedLimit function be handled? (Even if reset speed-limit fails, restore may still succeed.)

MoCuishle28 · 2022-06-06T02:59:17Z

/run-all-tests

MoCuishle28 · 2022-06-06T02:59:31Z

/run-integration-br-test

3pointer · 2022-06-06T10:24:37Z

Question:

Do We really need to reset the speed-limit of TiKV stores?

How should the error returned by the ResetSpeedLimit function be handled? (Even if reset speed-limit fails, restore may still succeed.)

I think we should handle reset speed-limit since we may have multiple restorations. and we can ignore the error after some times retry for now. in future we may need a mechanism to set speed limit in ttl. like what we do in switchmode

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

MoCuishle28 · 2022-06-10T06:28:46Z

/run-integration-br-test

MoCuishle28 · 2022-06-10T09:27:15Z

/run-integration-br-test

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

MoCuishle28 · 2022-06-12T06:29:04Z

/run-integration-br-test

MoCuishle28 · 2022-06-15T02:21:48Z

/cc @3pointer @joccau @YuJuncen PTAL
(BTW, please help me add labels as I don't have permission to add labels yet.)

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

@MoCuishle28: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @3pointer @joccau @YuJuncen PTAL
(BTW, please help me add labels as I don't have permission to add labels yet.)

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 kubernetes/test-infra repository.

br/pkg/restore/client.go

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

MoCuishle28 · 2022-06-28T05:09:38Z

/run-integration-br-test

3pointer · 2022-06-28T13:11:13Z

br/pkg/restore/client.go

+		}
+
 		opt := grpc.WithInsecure()
 		if rc.tlsConf != nil {


shall we move opt into workerPool?

Yes, we can move opt into workerPool. I have modified it. PTAL

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

MoCuishle28 · 2022-06-28T16:04:29Z

/run-integration-br-test

MoCuishle28 · 2022-06-29T01:54:12Z

/run-integration-br-test

3pointer

LGTM

3pointer · 2022-07-05T05:22:43Z

/merge

ti-chi-bot · 2022-07-05T05:22:47Z

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

Details

Commit hash: 36b55d6

ti-chi-bot · 2022-07-05T05:22:59Z

@MoCuishle28: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

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.

* upstream/master: (77 commits) infoschema: add a simply store for DM's SchemaTracker (pingcap#35954) planner: refactor "access object" related logic and prepare for binary plan (pingcap#35890) telemetry: Initialize telemetry asynchronously (pingcap#35898) sessionctx/variable: convert TiDBLogFileMaxDays and TiDBConfig to ScopeInstance (pingcap#35733) session, sessionctx/variable: ensure sysvars are always in lower case (pingcap#35659) *: add telemetry support for multi-schema change (pingcap#35977) topsql: compress plan instead of dropping it (pingcap#35973) *: use pdqsort to improve sort performance (pingcap#34404) ddl: refine GetDDLInfo function (pingcap#35969) *: Make the code cleaner in session.ExecutePreparedStmt (pingcap#35943) ddl: remove `onDropIndexes` (pingcap#35960) ddl: refine history job code (pingcap#35967) sessionctx,kv,planner: add system variable for fine_grained_shuffle (pingcap#35256) *: change remote cache (pingcap#35968) *: enable gosec (pingcap#35873) plancodec: fix a miss case in conversion (pingcap#35957) ddl: support drop indexes for multi-schema change (pingcap#35883) pdutil: fix unstable test TestPauseSchedulersByKeyRange (pingcap#35949) br: optimize communication with tikv stores. pingcap#31722 (pingcap#35147) txn: rename snapshot related methods in txnManager. (pingcap#35912) ...

br: optimize communication with tikv stores. pingcap#31722

719fe02

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 5, 2022

br: optimize communication with tikv stores. pingcap#31722

aa0aed2

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

Merge branch 'master' into issue-31722

db6c43a

br: optimize communication with tikv stores (pingcap#31722)

5e8dd53

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

ti-chi-bot requested review from 3pointer, YuJuncen and joccau June 15, 2022 02:21

YuJuncen reviewed Jun 28, 2022

View reviewed changes

br/pkg/restore/client.go Outdated Show resolved Hide resolved

YuJuncen added the component/br This issue is related to BR of TiDB. label Jun 28, 2022

br: optimize communication with tikv stores (pingcap#31722)

d225fe9

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

Merge branch 'master' into issue-31722

6a9a6f1

3pointer reviewed Jun 28, 2022

View reviewed changes

br: optimize communication with tikv stores. (pingcap#31722)

36b55d6

Signed-off-by: Gaoming Zhang <zhanggaoming028@gmail.com>

3pointer approved these changes Jun 30, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 30, 2022

MoCuishle28 requested a review from YuJuncen June 30, 2022 08:21

fengou1 approved these changes Jul 5, 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 Jul 5, 2022

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

Merge branch 'master' into issue-31722

eae890b

ti-chi-bot merged commit d8dbab2 into pingcap:master Jul 5, 2022

Conversation

MoCuishle28 commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoCuishle28 commented Jun 5, 2022

Uh oh!

MoCuishle28 commented Jun 6, 2022

Uh oh!

MoCuishle28 commented Jun 6, 2022

Uh oh!

3pointer commented Jun 6, 2022

Uh oh!

MoCuishle28 commented Jun 10, 2022

Uh oh!

MoCuishle28 commented Jun 10, 2022

Uh oh!

MoCuishle28 commented Jun 12, 2022

Uh oh!

MoCuishle28 commented Jun 15, 2022

Uh oh!

ti-chi-bot commented Jun 15, 2022

Uh oh!

Uh oh!

MoCuishle28 commented Jun 28, 2022

Uh oh!

3pointer Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

MoCuishle28 Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

MoCuishle28 commented Jun 28, 2022

Uh oh!

MoCuishle28 commented Jun 29, 2022

Uh oh!

3pointer left a comment

Choose a reason for hiding this comment

Uh oh!

3pointer commented Jul 5, 2022

Uh oh!

ti-chi-bot commented Jul 5, 2022

Uh oh!

ti-chi-bot commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MoCuishle28 commented Jun 5, 2022 •

edited

Loading

ti-chi-bot commented Jun 5, 2022 •

edited

Loading

sre-bot commented Jun 5, 2022 •

edited

Loading