storage: support online config for flow-control module by glorv · Pull Request #17396 · tikv/tikv

glorv · 2024-08-19T08:32:31Z

What is changed and how it works?

Issue Number: Close #17395

What's Changed:

support online config for flow-control module

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
Performance regression: Consumes more Memory
Breaking backward compatibility

Release note

Support online config change for flow-control related configs

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

LykxSassinator

LGTM

Connor1996

LGTM

ti-chi-bot · 2024-08-20T09:20:54Z

[LGTM Timeline notifier]

Timeline:

2024-08-19 09:34:18.739783385 +0000 UTC m=+171653.874233502: ☑️ agreed by LykxSassinator.
2024-08-20 09:20:53.733703226 +0000 UTC m=+257248.868153342: ☑️ agreed by Connor1996.

Connor1996 · 2024-08-20T09:20:56Z

/merge

ti-chi-bot · 2024-08-20T09:20:58Z

@Connor1996: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: Proposal: Strengthen configuration change approval.

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.

glorv · 2024-08-20T15:23:52Z

/approve

glorv · 2024-08-20T15:24:25Z

@zhangjinpeng87 PTAL

glorv · 2024-08-21T09:48:58Z

/retest

hbisheng · 2024-08-22T03:24:40Z

src/storage/txn/flow_controller/singleton_flow_controller.rs

        );
        let discard_ratio = Arc::new(AtomicU32::new(0));
-        let checker = FlowChecker::new(config, engine, discard_ratio.clone(), limiter.clone());
+        let config_tracker = Arc::new(VersionTrack::new(config.clone()));


Just trying to learn, what's the benefit of using a VersionTracker instead of a RwLock? Do we use the version anywhere?

There is no much difference here as we don't have any Tracker of this config.

hbisheng · 2024-08-22T03:41:25Z

src/storage/config_manager.rs

                }
                self.flow_controller.enable(enable);
            }
+            self.flow_controller.update_config(flow_control)?;


Should we call flow_controller.update_config(flow_control) before L79 (flow_controller.enable(enable)) ? As it is, I feel like there could be a small race condition and metric issue in the following scenario:

When we disable the flow controller, checker.reset_statistics() will be called first, but current_cfg.enable may still be true when on_flow_info_msg is called. So metrics like SCHED_PENDING_COMPACTION_BYTES_GAUGE may be non-zero after we disable the flow controller, which might be unexpected. Not sure if I understand all this correctly though.

ti-chi-bot · 2024-08-23T02:18:23Z

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

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.

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

zhangjinpeng87 · 2024-08-23T17:19:44Z

/approve

ti-chi-bot · 2024-08-23T17:19:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Connor1996, glorv, LykxSassinator, zhangjinpeng87

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Connor1996,LykxSassinator,glorv,zhangjinpeng87]
~~src/storage/OWNERS~~ [zhangjinpeng87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

close tikv#17395 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-06-05T08:58:21Z

In response to a cherrypick label: new pull request created to branch release-8.1: #18511.
But this PR has conflicts, please resolve them!

ti-chi-bot · 2025-06-05T08:58:21Z

In response to a cherrypick label: new pull request created to branch release-7.5: #18512.
But this PR has conflicts, please resolve them!

tikv#17396). Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

#17396). (#18523) Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Rollup of following sst ingestion optimizations * raftstore: directly write kvs rather than ingestion when merging small regions. (#17408) (#18518) * raftstore: remove stale ranges by DeleteByKeys rather than ingesting. (#18040) (#18519) * raftstore: support rocksdb writes during ingestion #18096 (#18520) * storage: support online config for flow-control module (#17396). (#18523) * sst_importer: allow write during ingesting sst (#18514) (#18522) Signed-off-by: lucasliang <nkcs_lykx@hotmail.com> Signed-off-by: Neil Shen <overvenus@gmail.com> Co-authored-by: lucasliang <nkcs_lykx@hotmail.com>

…ule (tikv#17396) Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

…ule (#17396) (#18539) close #17395 support online config for flow-control module Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

close #17395 support online config for flow-control module Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: lucasliang <nkcs_lykx@hotmail.com> Co-authored-by: glorv <glorvs@163.com> Co-authored-by: lucasliang <nkcs_lykx@hotmail.com>

support online config for flow-control module

d5333e6

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

glorv requested review from Connor1996 and LykxSassinator August 19, 2024 08:32

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 19, 2024

LykxSassinator approved these changes Aug 19, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 19, 2024

Connor1996 approved these changes Aug 20, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 20, 2024

ti-chi-bot bot added 2 commits August 20, 2024 09:21

Merge branch 'master' into flow-ctl-config

21271df

Merge branch 'master' into flow-ctl-config

6aa98a9

ti-chi-bot bot added 2 commits August 20, 2024 18:34

Merge branch 'master' into flow-ctl-config

baa5d4d

Merge branch 'master' into flow-ctl-config

93ca636

hbisheng reviewed Aug 22, 2024

View reviewed changes

ti-chi-bot bot added 3 commits August 22, 2024 06:48

Merge branch 'master' into flow-ctl-config

2ea04a2

Merge branch 'master' into flow-ctl-config

56a83a9

Merge branch 'master' into flow-ctl-config

e4c2633

glorv added 2 commits August 23, 2024 14:11

fix

ffece72

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

fmt code

288e8ff

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

ti-chi-bot bot added the approved label Aug 23, 2024

ti-chi-bot bot merged commit 444270c into tikv:master Aug 23, 2024

ti-chi-bot bot added this to the Pool milestone Aug 23, 2024

glorv deleted the flow-ctl-config branch September 24, 2024 08:20

LykxSassinator added needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. labels Jun 5, 2025

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Jun 5, 2025

This is an automated cherry-pick of tikv#17396

86d5766

close tikv#17395 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot pushed a commit to ti-chi-bot/tikv that referenced this pull request Jun 5, 2025

This is an automated cherry-pick of tikv#17396

0139f0c

close tikv#17395 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Jun 5, 2025

storage: support online config for flow-control module (#17396) #18511

Open

9 tasks

ti-chi-bot mentioned this pull request Jun 5, 2025

storage: support online config for flow-control module (#17396) #18512

Merged

9 tasks

LykxSassinator added a commit to LykxSassinator/tikv that referenced this pull request Jun 8, 2025

[hotfix-to-7.5] storage: support online config for flow-control module (

57a3a6f

tikv#17396). Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator mentioned this pull request Jun 8, 2025

[hotfix-to-7.5] storage: support online config for flow-control module (#17396). #18523

Merged

9 tasks

overvenus pushed a commit that referenced this pull request Jun 8, 2025

[hotfix-to-7.5] storage: support online config for flow-control module (

936c043

#17396). (#18523) Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator added a commit to LykxSassinator/tikv that referenced this pull request Jun 12, 2025

[hotfix-to-7.5.6] storage: support online config for flow-control mod…

ca538b1

…ule (tikv#17396) Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

LykxSassinator mentioned this pull request Jun 12, 2025

[hotfix-to-7.5.6] storage: support online config for flow-control module (#17396) #18539

Merged

9 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Jun 12, 2025

[hotfix-to-7.5.6] storage: support online config for flow-control mod…

256547e

…ule (#17396) (#18539) close #17395 support online config for flow-control module Signed-off-by: lucasliang <nkcs_lykx@hotmail.com>

Conversation

glorv commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

LykxSassinator left a comment

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 20, 2024

[LGTM Timeline notifier]

Uh oh!

Connor1996 commented Aug 20, 2024

Uh oh!

ti-chi-bot bot commented Aug 20, 2024

Uh oh!

glorv commented Aug 20, 2024

Uh oh!

glorv commented Aug 20, 2024

Uh oh!

glorv commented Aug 21, 2024

Uh oh!

hbisheng Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

glorv Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

hbisheng Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

glorv Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

Uh oh!

zhangjinpeng87 commented Aug 23, 2024

Uh oh!

ti-chi-bot bot commented Aug 23, 2024

Uh oh!

ti-chi-bot commented Jun 5, 2025

Uh oh!

ti-chi-bot commented Jun 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

glorv commented Aug 19, 2024 •

edited

Loading