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.

Sign up for GitHub

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

Jump to bottom

ttl: add resource group for ttl query #40930

Closed
wants to merge 1 commit into from
Closed

ttl: add resource group for ttl query #40930

wants to merge 1 commit into from

Conversation

YangKeao
Copy link
Member

@YangKeao YangKeao commented Feb 1, 2023

What problem does this PR solve?

Issue Number: close #40928

Problem Summary:

Add resource group tidb_ttl for the ttl query. The user can tune the configuration for this resource group through alter resource group.

I was running some experiments to get a more proper RU value.

What is changed and how it works?

Check List

Tests

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

WIP

Release note

Attach resource group for the TTL queries. The user can modify the priority and limitation of the TTL through configuring the resource group.

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

@ti-chi-bot
Copy link
Member

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 1, 2023
BornChanger
BornChanger reviewed Feb 1, 2023
View reviewed changes
session/bootstrap.go Show resolved Hide resolved
session/bootstrap.go Show resolved Hide resolved
session/bootstrap.go Show resolved Hide resolved
session/bootstrap.go Outdated Show resolved Hide resolved
@YangKeao
Copy link
Member Author

YangKeao commented Feb 2, 2023

/run-build-arm64

1 similar comment
@YangKeao
Copy link
Member Author

YangKeao commented Feb 2, 2023

/run-build-arm64

@YangKeao
Copy link
Member Author

YangKeao commented Feb 2, 2023

/run-build-arm64

@YangKeao YangKeao force-pushed the ttl-resource-limit branch 2 times, most recently from 377e60e to 6dfd0f9 Compare February 3, 2023 07:27
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2023
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2023
@YangKeao
Copy link
Member Author

YangKeao commented Feb 5, 2023

/run-build-arm64

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2023
@YangKeao YangKeao marked this pull request as ready for review February 6, 2023 07:14
@YangKeao YangKeao requested a review from a team as a code owner February 6, 2023 07:14
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2023
@YangKeao YangKeao requested a review from lcwangchao February 6, 2023 07:22
YangKeao
YangKeao commented Feb 6, 2023
View reviewed changes
session/bootstrap.go

// CreateTTLResourceGroup creates the resource group for ttl
// TODO: set a more proper value for the RU_PER_SECOND
CreateTTLResourceGroup = `CREATE RESOURCE GROUP IF NOT EXISTS tidb_ttl RU_PER_SEC=10000000`
Copy link
Member Author

Choose a reason for hiding this comment

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

I set a super big value for the tidb_ttl, so the ttl is not limited by default. If the users want to limit the TTL, they could alter the resource group tidb_ttl. Is it a good pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can user CREATE RESOURCE GROUP IF NOT EXISTS tidb_ttl RU_PER_SEC=10000 BURSTABLE instead.

@YangKeao YangKeao requested a review from glorv February 6, 2023 07:24
@glorv
Copy link
Contributor

glorv commented Feb 6, 2023 •
edited
Loading

/cc @bb7133 This seems to be a short term workaround for a specific propose and the implementation is somewhat tricky. Do we really need to merge it into master?

@YangKeao YangKeao marked this pull request as draft February 6, 2023 07:38
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2023
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2023
@YangKeao YangKeao marked this pull request as ready for review February 21, 2023 07:23
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2023
@YangKeao
Copy link
Member Author

/retest

@ti-chi-bot
Copy link
Member

@YangKeao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/unit-test 0d16e84 link true /test unit-test

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2023
@ti-chi-bot
Copy link
Member

@YangKeao: PR needs rebase.

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.

@YangKeao YangKeao closed this Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BornChanger BornChanger BornChanger left review comments

@lcwangchao lcwangchao Awaiting requested review from lcwangchao

@glorv glorv Awaiting requested review from glorv

Assignees
No one assigned
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add resource limitation for TTL queries.
4 participants
@YangKeao @ti-chi-bot @glorv @BornChanger