Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add write amp rate limiter #368

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

v01dstar
Copy link

@v01dstar v01dstar commented Sep 5, 2024

This PR are following PRs combined:

The WriteAmpBasedRateLimiter automatically adjusts rate limiting by predicting I/O trends using heuristic methods based on the write amplification factor. e.g. a high write throughput with HIGH priority (flush) implies, highly likely, there will be LOW priority write throughput soon, because RocksDB needs to compact the L0 files to lower levels, it is better to provision in advance. Compared to the default rate limiter, it reduces the likelihood of triggering write stalls and helps prevent I/O jitter.

Comments:
* Env::IOPriority has more options now, LOW, MID, HIGH, USER instead of just LOW and HIGH, write amplification rate limiter's tuning logic may need to adjust to this change.

@v01dstar v01dstar changed the base branch from 8.10.fb to 8.10.tikv September 6, 2024 06:11
@v01dstar
Copy link
Author

v01dstar commented Sep 6, 2024

/run-all-test

@purelind
Copy link

purelind commented Sep 6, 2024

/run-all-tests

3 similar comments
@purelind
Copy link

purelind commented Sep 6, 2024

/run-all-tests

@v01dstar
Copy link
Author

v01dstar commented Sep 6, 2024

/run-all-tests

@v01dstar
Copy link
Author

v01dstar commented Sep 6, 2024

/run-all-tests

@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 14, 2024
@v01dstar v01dstar marked this pull request as draft September 18, 2024 06:53
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2024
@v01dstar v01dstar marked this pull request as ready for review September 20, 2024 05:49
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 20, 2024
@v01dstar
Copy link
Author

@hbisheng

Signed-off-by: v01dstar <[email protected]>
Signed-off-by: Yang Zhang <[email protected]>
Copy link

@LykxSassinator LykxSassinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall LGTM

Copy link

ti-chi-bot bot commented Sep 23, 2024

@LykxSassinator: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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

Copy link

ti-chi-bot bot commented Sep 24, 2024

@SpadeA-Tang: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

@wuhuizuo
Copy link

wait for merging of #376.

@wuhuizuo
Copy link

/lgtm

refresh

Copy link

ti-chi-bot bot commented Sep 24, 2024

@wuhuizuo: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

refresh

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.

Copy link

ti-chi-bot bot commented Sep 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LykxSassinator, SpadeA-Tang, wuhuizuo

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [LykxSassinator,SpadeA-Tang]

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

Copy link

ti-chi-bot bot commented Sep 24, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-09-24 13:05:32.608226997 +0000 UTC m=+1571202.348650935: ✖️🔁 reset by ti-chi-bot.

@ti-chi-bot ti-chi-bot bot removed the lgtm label Sep 24, 2024
Copy link

ti-chi-bot bot commented Sep 24, 2024

New changes are detected. LGTM label has been removed.

@ti-chi-bot ti-chi-bot bot merged commit 192739d into tikv:8.10.tikv Sep 24, 2024
5 checks passed
@v01dstar v01dstar deleted the write-amp-rate-limiter branch September 24, 2024 17:17
@v01dstar
Copy link
Author

Merge #381 with this for future cherry-pick.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants