server: support dynamic config max-grpc-send-msg-len and raft-msg-max-batch-size #12335

glorv · 2022-04-07T09:21:46Z

Signed-off-by: glorv glorvs@163.com

What is changed and how it works?

Issue Number: Close #12334

What's Changed:

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch

Check List

Tests

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

Side effects

Performance regression
- Consumes more CPU
- Consumes more MEM
Breaking backward compatibility

Release note

None

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot · 2022-04-07T09:21:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

5kbpers
Connor1996

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.

glorv · 2022-04-07T10:00:34Z

/test

…msg-len

Signed-off-by: glorv <glorvs@163.com>

glorv · 2022-05-16T04:15:18Z

@5kbpers @Connor1996 PTAL

Signed-off-by: glorv <glorvs@163.com>

…msg-len

5kbpers · 2022-05-16T09:05:24Z

CI failed

glorv · 2022-05-16T09:21:12Z

/test

glorv · 2022-05-16T09:22:20Z

CI failed

All failures are caused by github connection timeout.😂️

glorv · 2022-05-16T09:22:41Z

/build

Connor1996 · 2022-05-16T09:43:43Z

src/server/raft_client.rs

-        for entry in msg.get_message().get_entries() {
-            msg_size += entry.data.len();
+        let msg_size = Self::message_size(&msg);
+        // try refrech config before check


refrech -> refresh

Connor1996 · 2022-05-16T09:45:50Z

src/server/server.rs

@@ -163,7 +163,7 @@ impl<T: RaftStoreRouter<E::Local> + Unpin, S: StoreAddrResolver + 'static, E: En

        let conn_builder = ConnectionBuilder::new(
            env.clone(),
-            Arc::new(cfg.value().clone()),
+            Arc::clone(cfg),


cfg.clone()

I'm not sure either Arc::clone or .close() is recommended in our code style, though both are ok to me. There are both of them in the current codebase. (Search in tikv for Arc::clone returns 333 hits)

Connor1996

rest LGTM

src/server/raft_client.rs

Signed-off-by: glorv <glorvs@163.com>

…o grpc-send-msg-len

Signed-off-by: glorv <glorvs@163.com>

glorv · 2022-05-17T07:00:28Z

@Connor1996 PTAL again, thanks.

Connor1996

LGTM

5kbpers · 2022-05-18T02:47:39Z

/merge

ti-chi-bot · 2022-05-18T02:47:39Z

@5kbpers: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

ti-chi-bot · 2022-05-18T02:47:41Z

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

Commit hash: a3be3ff

…-batch-size (tikv#12335) close tikv#12334 Signed-off-by: glorv <glorvs@163.com>

BusyJay · 2022-06-02T22:07:14Z

components/raftstore/src/store/config.rs

@@ -461,6 +460,12 @@ impl Config {
            ));
        }

+        if self.raft_entry_max_size.0 == 0 || self.raft_entry_max_size.0 > ReadableSize::gb(3).0 {
+            return Err(box_err!(
+                "raft entry max size should be greater than 0 and less than or equal to 3GiB"


I don't think we support 3GiB. Not even 2.01GiB.

I set this to the vlaue as raft-max-size-per-msg, this is just an intuitive upper bound, do you mean the hard limit should be 2GB? Sorry I don't find the related code.

BusyJay · 2022-06-02T22:08:11Z

src/server/raft_client.rs

-            msg_size += entry.data.len();
+        let msg_size = Self::message_size(&msg);
+        // try refresh config before check
+        if let Some(new_cfg) = self.cfg_tracker.any_new() {


Have you bench the performance impact?

In my benchmark, this is no notable impact after this change. In the most common case, this is only one extra atomic load.

Actually the atomic load is way heavier than the original that just sums up several numbers. A better place to check for new version is when a message is sent.

So do you mean that we move this check into flush call as the frequency of flush should be much lower than push especially when the lead is heavy.

BusyJay · 2022-06-02T22:13:11Z

src/server/config.rs

@@ -83,7 +83,6 @@ pub struct Config {
    #[online_config(skip)]
    pub status_thread_pool_size: usize,

-    #[online_config(skip)]


I don't see the point of changing this value online. It's only for controlling the size of a batch.

The use case is from tidb-cloud to minimize the memory usage in a minimal test cluster. This kind of scenario is not common. I think it is harmless to support it.

The batch is about 10MiB, and it only consumes about 10MiB * grpc_concurrency = 40MiB.

support online config grpc-send-msg-len and raft-msg-max-batch-size

201f48e

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot added release-note-none size/L labels Apr 7, 2022

glorv requested review from 5kbpers and NingLin-P April 7, 2022 09:22

ti-chi-bot added the needs-rebase label Apr 27, 2022

Merge branch 'master' of https://github.com/tikv/tikv into grpc-send-…

f8a4a1c

…msg-len

ti-chi-bot removed the needs-rebase label May 16, 2022

also support raft-entry-max-size

8e4cf21

Signed-off-by: glorv <glorvs@163.com>

glorv added 3 commits May 16, 2022 13:35

fix

dcf481d

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'master' of https://github.com/tikv/tikv into grpc-send-…

7fa12bc

…msg-len

Merge branch 'master' into grpc-send-msg-len

6fe27d4

5kbpers approved these changes May 16, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Status: PR - There is already 1 approval label May 16, 2022

Merge branch 'master' into grpc-send-msg-len

a87f5b5

Connor1996 reviewed May 16, 2022

View reviewed changes

glorv commented May 16, 2022

View reviewed changes

src/server/raft_client.rs Outdated Show resolved Hide resolved

fix typo

8f918fa

Signed-off-by: glorv <glorvs@163.com>

glorv force-pushed the grpc-send-msg-len branch from b5a2a2e to 8f918fa Compare May 16, 2022 09:48

glorv added 2 commits May 16, 2022 19:22

Merge branch 'master' into grpc-send-msg-len

536e3d2

Merge branch 'master' into grpc-send-msg-len

7cf524f

glorv added 7 commits May 16, 2022 21:04

fmt code

cb6f8ae

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'grpc-send-msg-len' of https://github.com/glorv/tikv int…

09d00a1

…o grpc-send-msg-len

fix test

e6dfab3

Signed-off-by: glorv <glorvs@163.com>

check max-grpc-send-msg-len

85e91a3

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'master' into grpc-send-msg-len

3760888

fix unit test

f683712

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'master' into grpc-send-msg-len

8614a67

Connor1996 approved these changes May 17, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels May 17, 2022

Merge branch 'master' into grpc-send-msg-len

a3be3ff

ti-chi-bot added the status/can-merge Status: Can merge to base branch label May 18, 2022

ti-chi-bot merged commit 490c93d into tikv:master May 18, 2022

fengou1 pushed a commit to fengou1/tikv that referenced this pull request May 26, 2022

server: support dynamic config max-grpc-send-msg-len and raft-msg-max…

895e597

…-batch-size (tikv#12335) close tikv#12334 Signed-off-by: glorv <glorvs@163.com>

BusyJay reviewed Jun 2, 2022

View reviewed changes

glorv deleted the grpc-send-msg-len branch June 6, 2022 02:00

glorv mentioned this pull request Jun 8, 2022

Optimize the config change check in raft_client MessageBuffer #12780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: support dynamic config max-grpc-send-msg-len and raft-msg-max-batch-size #12335

server: support dynamic config max-grpc-send-msg-len and raft-msg-max-batch-size #12335

glorv commented Apr 7, 2022

ti-chi-bot commented Apr 7, 2022 •

edited

glorv commented Apr 7, 2022

glorv commented May 16, 2022

5kbpers commented May 16, 2022

glorv commented May 16, 2022

glorv commented May 16, 2022

glorv commented May 16, 2022

Connor1996 May 16, 2022

Connor1996 May 16, 2022

glorv May 16, 2022

Connor1996 left a comment

glorv commented May 17, 2022

Connor1996 left a comment

5kbpers commented May 18, 2022

ti-chi-bot commented May 18, 2022

ti-chi-bot commented May 18, 2022

BusyJay Jun 2, 2022

glorv Jun 6, 2022

BusyJay Jun 2, 2022

glorv Jun 6, 2022

BusyJay Jun 6, 2022

glorv Jun 7, 2022

BusyJay Jun 7, 2022

BusyJay Jun 2, 2022

glorv Jun 6, 2022

BusyJay Jun 6, 2022

server: support dynamic config max-grpc-send-msg-len and raft-msg-max-batch-size #12335

server: support dynamic config max-grpc-send-msg-len and raft-msg-max-batch-size #12335

Conversation

glorv commented Apr 7, 2022

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot commented Apr 7, 2022 • edited

glorv commented Apr 7, 2022

glorv commented May 16, 2022

5kbpers commented May 16, 2022

glorv commented May 16, 2022

glorv commented May 16, 2022

glorv commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

glorv commented May 17, 2022

Connor1996 left a comment

Choose a reason for hiding this comment

5kbpers commented May 18, 2022

ti-chi-bot commented May 18, 2022

ti-chi-bot commented May 18, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Apr 7, 2022 •

edited