parser,ddl: add background job syntax and meta management for resource control by glorv · Pull Request #45189 · pingcap/tidb

glorv · 2023-07-05T09:51:48Z

What problem does this PR solve?

Issue Number: ref #44517

Problem Summary:

What is changed and how it works?

Resource control add new syntax for background tasks management as following:

// Set background tasks
CREATE RESOURCE GROUP `rg1` ru_per_sec = 1000 BACKGROUND=(TASK_NAMES="br,lightning");
// change settings
ALTER RESOURCE GROUP `rg1` BACKGROUND = (TASK_NAMES="br");
// reset background tasks
ALTER RESOURCE GROUP `rg1` BACKGROUND = NULL;

NOTE: in the first phase, we only support changing the default resource group's background config.

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2023-07-05T09:52:16Z

Hi @glorv. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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

CabinfeverB · 2023-07-06T09:10:33Z

parser/parser.y

+	}
+
+DirectResourceGroupBackgroundOption:
+	"TASK_NAMES" EqOpt stringLit


Is the task name an enumeration type? I find PD and TiDB won't check whether name is valid, user can't get feedback if the name is wrong.

Yes, you can check the enum here. @CabinfeverB
https://github.com/tikv/client-go/pull/868/files#diff-6bd1a8e867c588123e135e97a8474639b648d9b6e4ab28c5b29338b388505dfbR41

My main concern is that when setting the backgroup, if the user input is wrong, it will not report an error.

Added a check in ddl api

…und-meta

glorv · 2023-07-10T03:58:07Z

@CabinfeverB @HuSharp @nolouch PTAL again, thanks

nolouch

lgtm

HuSharp · 2023-07-10T09:40:27Z

ddl/ddl_api.go

+			}
+		}
+	case ast.ResourceGroupBackground:
+		if groupInfo.Name.L != rg.DefaultResourceGroupName {


I noticed this restriction, so maybe need to update pr description?

There is a NOTE in the PR description.

oops, sorrrry! I must be blind!

HuSharp · 2023-07-10T09:55:33Z

parser/ast/ddl.go

+					ctx.WritePlain(", ")
+				}
+				if err := option.Restore(ctx); err != nil {
+					return errors.Annotatef(err, "An error occurred while splicing ResourceGroup Background Option")


Do we need to unify error with ResourceGroupRunaway to expose i?

Suggested change

return errors.Annotatef(err, "An error occurred while splicing ResourceGroup Background Option")

return errors.Annotatef(err, "An error occurred while splicing ResourceGroup Background Option: [%v]", i")

HuSharp · 2023-07-10T09:55:44Z

parser/ast/ddl.go

+					ctx.WritePlain(" ")
+				}
+				if err := option.Restore(ctx); err != nil {
+					return errors.Annotatef(err, "An error occurred while splicing CreateResourceGroupStmt Option: [%v]", i)


Suggested change

return errors.Annotatef(err, "An error occurred while splicing CreateResourceGroupStmt Option: [%v]", i)

return errors.Annotatef(err, "An error occurred while splicing ResourceGroupRunaway Option: [%v]", i)

HuSharp

merge master failed, rest LGTM!

ti-chi-bot · 2023-07-11T06:07:40Z

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

Details

In response to this:

merge master failed, reset 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.

…und-meta

ti-chi-bot · 2023-07-11T07:16:15Z

@glorv: 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
pull-br-integration-test	`fe020f3`	link	true	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

glorv · 2023-07-11T08:00:09Z

/retest

tiprow · 2023-07-11T08:00:52Z

@glorv: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

tiprow · 2023-07-11T08:05:14Z

@glorv: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test check-dev2

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.

ti-chi-bot · 2023-07-11T09:01:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, HuSharp, nolouch

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~~ [3pointer]

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

ti-chi-bot · 2023-07-11T09:01:11Z

[LGTM Timeline notifier]

Timeline:

2023-07-10 09:17:53.274943992 +0000 UTC m=+323565.045282708: ☑️ agreed by nolouch.
2023-07-11 09:01:10.064715895 +0000 UTC m=+408961.835054607: ☑️ agreed by 3pointer.

hawkingrei · 2023-07-11T09:03:24Z

/hold

tiprow · 2023-07-11T09:06:16Z

@glorv: 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
tiprow_fast_test	`7e5236f`	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

hawkingrei · 2023-07-11T09:09:56Z

/unhold

supprot resource group background parser and meta

6252610

glorv requested review from BornChanger, CabinfeverB, HuSharp and nolouch July 5, 2023 09:51

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 5, 2023

glorv added 2 commits July 5, 2023 18:02

fix lint

105b7e0

fix build

fe020f3

ti-chi-bot bot deleted a comment from ti-chi-bot Jul 5, 2023

CabinfeverB reviewed Jul 6, 2023

View reviewed changes

validate resource group background task names

b81c599

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

ti-chi-bot bot deleted a comment from ti-chi-bot Jul 10, 2023

Merge branch 'master' of https://github.com/pingcap/tidb into backgro…

f82601a

…und-meta

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2023

ti-chi-bot bot deleted a comment from ti-chi-bot Jul 10, 2023

nolouch approved these changes Jul 10, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 10, 2023

HuSharp reviewed Jul 10, 2023

View reviewed changes

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

fix

0a6c2fd

HuSharp approved these changes Jul 11, 2023

View reviewed changes

Merge branch 'master' of https://github.com/pingcap/tidb into backgro…

57b05a8

…und-meta

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2023

fix bazel

7e5236f

3pointer approved these changes Jul 11, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Jul 11, 2023

ti-chi-bot bot added approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 11, 2023

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023

remove useless dep

2a0b3ea

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023

ti-chi-bot bot merged commit 43763d2 into pingcap:master Jul 11, 2023

glorv deleted the background-meta branch July 11, 2023 09:59

tangenta mentioned this pull request Jul 12, 2023

parser: change token TASK_NAMES to TASK_TYPES #45331

Merged

12 tasks

glorv mentioned this pull request Jun 26, 2023

Tracking Issue of Background Resource Control #44517

Closed

14 tasks

	return errors.Annotatef(err, "An error occurred while splicing ResourceGroup Background Option")
	return errors.Annotatef(err, "An error occurred while splicing ResourceGroup Background Option: [%v]", i")

Conversation

glorv commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

tiprow bot commented Jul 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuSharp Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glorv commented Jul 10, 2023

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuSharp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Jul 11, 2023

Uh oh!

ti-chi-bot bot commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glorv commented Jul 11, 2023

Uh oh!

tiprow bot commented Jul 11, 2023

Uh oh!

tiprow bot commented Jul 11, 2023

Uh oh!

ti-chi-bot bot commented Jul 11, 2023

Uh oh!

ti-chi-bot bot commented Jul 11, 2023

[LGTM Timeline notifier]

Uh oh!

hawkingrei commented Jul 11, 2023

Uh oh!

tiprow bot commented Jul 11, 2023

Uh oh!

hawkingrei commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

glorv commented Jul 5, 2023 •

edited

Loading

HuSharp Jul 6, 2023 •

edited

Loading

HuSharp left a comment •

edited

Loading

ti-chi-bot bot commented Jul 11, 2023 •

edited

Loading