Skip to content

resource_control: support manage resource group background metadata#14921

Merged
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
glorv:bg-meta
Jun 14, 2023
Merged

resource_control: support manage resource group background metadata#14921
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
glorv:bg-meta

Conversation

@glorv
Copy link
Contributor

@glorv glorv commented Jun 12, 2023

What is changed and how it works?

Issue Number: ref #14900

What's Changed:

- Add a new type `ResourceLimiter` to limit the cpu and io resource usage.
- Add ResourceLimiter to ResourceGroup as an optional field, currently, only the `default` resource group contains this field, we may support other groups later.

Related changes

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

Check List

Tests

  • Unit test

Side effects

Release note

None

Signed-off-by: glorv <glorvs@163.com>
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 12, 2023

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Connor1996
  • nolouch

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.

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 12, 2023
@glorv glorv marked this pull request as ready for review June 12, 2023 09:43
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 12, 2023
@glorv glorv requested review from Connor1996 and nolouch June 12, 2023 09:43
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
default_group.name = DEFAULT_RESOURCE_GROUP_NAME.into();
default_group.priority = MEDIUM_PRIORITY;
default_group.mode = GroupMode::RuMode;
default_group
Copy link
Contributor

Choose a reason for hiding this comment

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

the default_group will create in pd side, so tikv will automatically add the group. why do we need add this?

Copy link
Contributor Author

@glorv glorv Jun 13, 2023

Choose a reason for hiding this comment

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

The ResourceManage depend on the default group's exists to handle fallback logic. Since the watch pd logic is async, it's possible that the initialization don't finish before some task has come.
You can see in the previous version, we inited the default group at the ResouceController level, but in order to keep the ResourceLimiter when we update the cfg of resource group, I changed this init at ResourceGroupManager level, but the logic is still the same as before.

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

impl ResourceGroupManager {
fn get_ru_setting(rg: &ResourceGroup, is_read: bool) -> u64 {
#[allow(clippy::new_without_default)]
Copy link
Member

Choose a reason for hiding this comment

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

why not implement default

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

rest LGTM

glorv added 3 commits June 14, 2023 10:50
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
@glorv
Copy link
Contributor Author

glorv commented Jun 14, 2023

@nolouch @Connor1996 PTAL again, thanks~

@glorv
Copy link
Contributor Author

glorv commented Jun 14, 2023

/test

1 similar comment
@glorv
Copy link
Contributor Author

glorv commented Jun 14, 2023

/test

Copy link
Contributor

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 14, 2023
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 14, 2023
@Connor1996
Copy link
Member

/merge

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 14, 2023

@Connor1996: 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.

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.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 14, 2023

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

DetailsCommit hash: 2b2cd9a

@ti-chi-bot ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 14, 2023
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 14, 2023

@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.

@ti-chi-bot ti-chi-bot bot merged commit 160d3d2 into tikv:master Jun 14, 2023
@ti-chi-bot ti-chi-bot bot added this to the Pool milestone Jun 14, 2023
tonyxuqqi pushed a commit to tonyxuqqi/tikv that referenced this pull request Jun 22, 2023
…ikv#14921)

ref tikv#14900

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

Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>
Signed-off-by: tonyxuqqi <tonyxuqi@outlook.com>
@glorv glorv deleted the bg-meta branch July 28, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants