resource_manager/client: resource group Independent reporting time #6150

CabinfeverB · 2023-03-12T15:29:44Z

What problem does this PR solve?

Issue Number: Close #6151 Close #6113

What is changed and how does it work?

The following changes have been made:

remove most fields inResourceGroupsController and introduce requestFailed and lastRequestTime into groupCostController .
Every time the stateUpdateTicker check is triggered, all resource groups are traversed to see if they need to be reported. There are three conditions for the report: low token, RU consumption in the period exceeds the threshold, and the last request fails.

Check List

Tests

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

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2023-03-12T15:29:45Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-03-12T15:29:46Z

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

codecov · 2023-03-12T15:40:44Z

Codecov Report

Patch coverage: 57.50% and project coverage change: -0.05 ⚠️

Comparison is base (0b36238) 74.41% compared to head (1f0627c) 74.37%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6150      +/-   ##
==========================================
- Coverage   74.41%   74.37%   -0.05%     
==========================================
  Files         391      391              
  Lines       38175    38161      -14     
==========================================
- Hits        28409    28382      -27     
- Misses       7241     7265      +24     
+ Partials     2525     2514      -11

Flag	Coverage Δ
unittests	`74.37% <57.50%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
client/resource_group/controller/config.go	`100.00% <ø> (ø)`
client/resource_group/controller/controller.go	`61.03% <57.50%> (-3.82%)`	⬇️

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

nolouch

rest lgtm

nolouch · 2023-03-13T10:16:39Z

client/resource_group/controller/controller.go

-					c.run.requestNeedsRetry = false
-					c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, false /* select all */)
+				if len(c.run.currentRequests) == 0 {
+					c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, 0 /* select resource groups which should be reported periodically */)


Suggested change

c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, 0 /* select resource groups which should be reported periodically */)

c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, periodicReport /* select resource groups which should be reported periodically */)

nolouch · 2023-03-13T10:16:53Z

client/resource_group/controller/controller.go

-				if !c.run.requestInProgress {
-					c.collectTokenBucketRequests(c.loopCtx, FromLowRU, true /* only select low tokens resource group */)
+				if len(c.run.currentRequests) == 0 {
+					c.collectTokenBucketRequests(c.loopCtx, FromLowRU, 1 /* select low tokens resource group */)


Suggested change

c.collectTokenBucketRequests(c.loopCtx, FromLowRU, 1 /* select low tokens resource group */)

c.collectTokenBucketRequests(c.loopCtx, FromLowRU, lowToken /* select low tokens resource group */)

nolouch · 2023-03-13T10:18:41Z

ptal @JmPotato @HuSharp

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato

The rest LGTM.

JmPotato · 2023-03-14T03:16:54Z

client/resource_group/controller/controller.go

@@ -88,20 +95,7 @@ type ResourceGroupsController struct {
 	tokenBucketUpdateChan chan *groupCostController

 	run struct {


May I suggest removing the redundant field as it appears to only contain a single value?

JmPotato · 2023-03-14T03:19:28Z

client/resource_group/controller/controller.go

+		// requestFailed is set true when sending token bucket request.
+		// And it is set false when reciving token bucket response.
+		// This triggers a retry attempt on the next tick.
+		requestFailed bool


The usage looks not very related to the "fail", requestInProgress makes more sence to me.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2023-03-14T03:55:38Z

/merge

ti-chi-bot · 2023-03-14T03:55:39Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 · 2023-03-14T03:55:42Z

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

Commit hash: 691c2aa

ti-chi-bot · 2023-03-14T03:55:54Z

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

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.

self report

1477e73

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added the do-not-merge/needs-linked-issue label Mar 12, 2023

ti-chi-bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 12, 2023

ti-chi-bot requested review from HunDunDM and lhy1024 March 12, 2023 15:29

ti-chi-bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Mar 13, 2023

CabinfeverB requested review from nolouch and JmPotato and removed request for lhy1024 and HunDunDM March 13, 2023 02:01

CabinfeverB changed the title ~~self report~~ resource_manager/client: resource group Independent reporting time Mar 13, 2023

CabinfeverB marked this pull request as ready for review March 13, 2023 02:34

ti-chi-bot removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/needs-triage-completed labels Mar 13, 2023

nolouch approved these changes Mar 13, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 13, 2023

address comment

1fe08d4

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato reviewed Mar 14, 2023

View reviewed changes

address comment

691c2aa

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato approved these changes Mar 14, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 14, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 14, 2023

Merge branch 'master' into resource-manager/report_period

1f0627c

ti-chi-bot merged commit 163c08c into tikv:master Mar 14, 2023
17 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager/client: resource group Independent reporting time #6150

resource_manager/client: resource group Independent reporting time #6150

CabinfeverB commented Mar 12, 2023 •

edited

ti-chi-bot commented Mar 12, 2023 •

edited

ti-chi-bot commented Mar 12, 2023

codecov bot commented Mar 12, 2023 •

edited

nolouch left a comment

nolouch Mar 13, 2023

nolouch Mar 13, 2023

nolouch commented Mar 13, 2023

JmPotato left a comment

JmPotato Mar 14, 2023

JmPotato Mar 14, 2023

JmPotato commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

	c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, 0 /* select resource groups which should be reported periodically */)
	c.collectTokenBucketRequests(c.loopCtx, FromPeriodReport, periodicReport /* select resource groups which should be reported periodically */)

		@@ -88,20 +95,7 @@ type ResourceGroupsController struct {
		tokenBucketUpdateChan chan *groupCostController

		run struct {

resource_manager/client: resource group Independent reporting time #6150

resource_manager/client: resource group Independent reporting time #6150

Conversation

CabinfeverB commented Mar 12, 2023 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 12, 2023 • edited

ti-chi-bot commented Mar 12, 2023

codecov bot commented Mar 12, 2023 • edited

Codecov Report

nolouch left a comment

Choose a reason for hiding this comment

nolouch Mar 13, 2023

Choose a reason for hiding this comment

nolouch Mar 13, 2023

Choose a reason for hiding this comment

nolouch commented Mar 13, 2023

JmPotato left a comment

Choose a reason for hiding this comment

JmPotato Mar 14, 2023

Choose a reason for hiding this comment

JmPotato Mar 14, 2023

Choose a reason for hiding this comment

JmPotato commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

ti-chi-bot commented Mar 14, 2023

CabinfeverB commented Mar 12, 2023 •

edited

ti-chi-bot commented Mar 12, 2023 •

edited

codecov bot commented Mar 12, 2023 •

edited