resource_manager/client: introduce watch resource group #6510

CabinfeverB · 2023-05-24T13:30:58Z

What problem does this PR solve?

Issue Number: ref #6509

What is changed and how does it work?

In the past, we didn't need to update the configuration of the group in a timely manner because when we only had the token bucket function, we only needed to get tokens from the server. But after joining the runaway query, we need to get a new configuration in time.

Check List

Tests

Integration test

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

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

Release note

None.

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

ti-chi-bot · 2023-05-24T13:31:01Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Connor1996
HuSharp

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.

CabinfeverB · 2023-05-24T13:31:28Z

cc @HuSharp @JmPotato

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> add watch resource group Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

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

codecov · 2023-05-25T02:00:19Z

Codecov Report

Patch coverage: 71.27% and project coverage change: -0.01 ⚠️

Comparison is base (ad05acb) 74.69% compared to head (d2f90ea) 74.69%.

❗ Current head d2f90ea differs from pull request most recent head f47c9f8. Consider uploading reports for the commit f47c9f8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6510      +/-   ##
==========================================
- Coverage   74.69%   74.69%   -0.01%     
==========================================
  Files         415      415              
  Lines       42494    42556      +62     
==========================================
+ Hits        31740    31786      +46     
- Misses       7966     7974       +8     
- Partials     2788     2796       +8

Flag	Coverage Δ
unittests	`74.69% <71.27%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
client/errs/errno.go	`100.00% <ø> (ø)`
client/meta_storage_client.go	`66.99% <50.00%> (-1.49%)`	⬇️
client/resource_manager_client.go	`70.42% <56.25%> (-2.85%)`	⬇️
client/resource_group/controller/controller.go	`75.16% <77.77%> (+0.75%)`	⬆️
pkg/mcs/metastorage/server/grpc_service.go	`64.10% <100.00%> (+2.28%)`	⬆️

... and 34 files with indirect coverage changes

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

CabinfeverB · 2023-05-25T02:19:02Z

client/meta_storage_client.go

@@ -158,7 +163,12 @@ func (c *client) Get(ctx context.Context, key []byte, opts ...OpOption) (*meta_s
 		Revision: options.revision,
 	}
 	ctx = grpcutil.BuildForwardContext(ctx, c.GetLeaderAddr())
-	resp, err := c.metaStorageClient().Get(ctx, req)
+	cli := c.metaStorageClient()


Please help take a look here, thx. @rleungx @binshi-bing

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

HuSharp · 2023-05-26T02:52:20Z

client/resource_group/controller/controller.go

 	GetResourceGroup(ctx context.Context, resourceGroupName string) (*rmpb.ResourceGroup, error)
 	AddResourceGroup(ctx context.Context, metaGroup *rmpb.ResourceGroup) (string, error)
 	ModifyResourceGroup(ctx context.Context, metaGroup *rmpb.ResourceGroup) (string, error)
 	DeleteResourceGroup(ctx context.Context, resourceGroupName string) (string, error)
 	AcquireTokenBuckets(ctx context.Context, request *rmpb.TokenBucketsRequest) ([]*rmpb.TokenBucketResponse, error)
 	LoadGlobalConfig(ctx context.Context, names []string, configPath string) ([]pd.GlobalConfigItem, int64, error)
+	LoadResourcrGroups(ctx context.Context) ([]*rmpb.ResourceGroup, int64, error)
+	Watch(ctx context.Context, key []byte, opts ...pd.OpOption) (chan []*meta_storagepb.Event, error)


Do we need to keep consistent with ResourceGroupClient?

Can we can add the Watch into ResourceGroupClient interface?

HuSharp · 2023-05-26T02:52:43Z

client/resource_manager_client.go

+	}
+	return groups, resp.Header.Revision, nil
+}
+
 // WatchResourceGroup [just for TEST] watches resource groups changes.


Can remove [just for TEST]

This function is still used for testing purposes

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

Connor1996 · 2023-05-29T08:49:51Z

client/resource_group/controller/controller.go

 	GetResourceGroup(ctx context.Context, resourceGroupName string) (*rmpb.ResourceGroup, error)
 	AddResourceGroup(ctx context.Context, metaGroup *rmpb.ResourceGroup) (string, error)
 	ModifyResourceGroup(ctx context.Context, metaGroup *rmpb.ResourceGroup) (string, error)
 	DeleteResourceGroup(ctx context.Context, resourceGroupName string) (string, error)
 	AcquireTokenBuckets(ctx context.Context, request *rmpb.TokenBucketsRequest) ([]*rmpb.TokenBucketResponse, error)
 	LoadGlobalConfig(ctx context.Context, names []string, configPath string) ([]pd.GlobalConfigItem, int64, error)
+	LoadResourcrGroups(ctx context.Context) ([]*rmpb.ResourceGroup, int64, error)


Resourcr -> Resource

Connor1996

LGTM

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

Connor1996 · 2023-05-31T06:36:44Z

client/resource_group/controller/controller.go

-	for _, group := range groups {
-		latestGroups[group.GetName()] = struct{}{}
-	}
+func (c *ResourceGroupsController) cleanUpResourceGroup() {


help add a GetResourceGrouup() by the way

HuSharp

rest LGTM!

HuSharp · 2023-05-31T12:28:25Z

client/resource_group/controller/controller.go

@@ -732,7 +773,7 @@ func (gc *groupCostController) updateAvgRUPerSec() {
 		if !gc.calcAvg(counter, getRUValueFromConsumption(gc.run.consumption, typ)) {
 			continue
 		}
-		log.Debug("[resource group controller] update avg ru per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))
+		log.Debug("[resource group controller] update avg ru per sec", zap.String("name", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))


HuSharp · 2023-05-31T12:28:29Z

client/resource_group/controller/controller.go

@@ -718,7 +759,7 @@ func (gc *groupCostController) updateAvgRaWResourcePerSec() {
 		if !gc.calcAvg(counter, getRawResourceValueFromConsumption(gc.run.consumption, typ)) {
 			continue
 		}
-		log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))
+		log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))


Suggested change

log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))

log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avg-ru-per-sec", counter.avgRUPerSec))

Do we need to keep consistent with this https://github.com/tikv/pd/pull/6450/files#diff-ed72d626078622ff5c8e9b3f7d1876c4b3d6c648a4bc833785ca312da718aa07R319

HuSharp · 2023-05-31T12:29:46Z

client/resource_group/controller/controller.go

@@ -837,7 +878,7 @@ func (gc *groupCostController) applyBasicConfigForRUTokenCounters() {
 			cfg.NewRate = 99999999
 		})
 		counter.limiter.Reconfigure(gc.run.now, cfg, resetLowProcess())
-		log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource group", gc.Name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))
+		log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource group", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))


Suggested change

log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource group", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))

log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource-group", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))

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

CabinfeverB · 2023-05-31T13:59:30Z

/merge

ti-chi-bot · 2023-05-31T13:59:31Z

@CabinfeverB: 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-05-31T13:59:33Z

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

Commit hash: d2f90ea

ti-chi-bot · 2023-05-31T13:59:46Z

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

ref tikv#6509 Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

add watch resource group

6e2e6d4

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

CabinfeverB requested review from nolouch and Connor1996 May 24, 2023 13:30

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 24, 2023

add watch resource group

3d9535d

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> add watch resource group Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB force-pushed the resource_manager/refactor_watch branch from 76a5cbf to 3d9535d Compare May 24, 2023 13:37

CabinfeverB added 3 commits May 24, 2023 21:57

address comment

483edd6

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

address comment

e30df4f

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

address comment

4a7bb44

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

CabinfeverB commented May 25, 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 May 25, 2023

merge master

b74ce2e

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

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

HuSharp reviewed May 26, 2023

View reviewed changes

address comment

e17aaec

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

Connor1996 mentioned this pull request May 29, 2023

Runaway query control based on resource group pingcap/tidb#43691

Closed

28 tasks

Connor1996 reviewed May 29, 2023

View reviewed changes

Connor1996 approved these changes May 29, 2023

View reviewed changes

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

address comment

0516aae

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

Connor1996 reviewed May 31, 2023

View reviewed changes

HuSharp approved these changes May 31, 2023

View reviewed changes

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 May 31, 2023

address comment

d2f90ea

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

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

Merge branch 'master' into resource_manager/refactor_watch

f47c9f8

ti-chi-bot bot merged commit 9d0249c into tikv:master May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager/client: introduce watch resource group #6510

resource_manager/client: introduce watch resource group #6510

CabinfeverB commented May 24, 2023

ti-chi-bot bot commented May 24, 2023 •

edited

Loading

CabinfeverB commented May 24, 2023

codecov bot commented May 25, 2023 •

edited

Loading

CabinfeverB May 25, 2023

HuSharp May 26, 2023

CabinfeverB May 26, 2023

HuSharp May 26, 2023

CabinfeverB May 26, 2023

Connor1996 May 29, 2023

Connor1996 left a comment

Connor1996 May 31, 2023

HuSharp left a comment

HuSharp May 31, 2023

HuSharp May 31, 2023

HuSharp May 31, 2023

CabinfeverB commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

	log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))
	log.Debug("[resource group controller] update avg raw resource per sec", zap.String("name", gc.name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avg-ru-per-sec", counter.avgRUPerSec))

	log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource group", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))
	log.Info("[resource group controller] resource token bucket enter degraded mode", zap.String("resource-group", gc.name), zap.String("type", rmpb.RequestUnitType_name[int32(typ)]))

resource_manager/client: introduce watch resource group #6510

resource_manager/client: introduce watch resource group #6510

Conversation

CabinfeverB commented May 24, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented May 24, 2023 • edited Loading

CabinfeverB commented May 24, 2023

codecov bot commented May 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CabinfeverB commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

ti-chi-bot bot commented May 31, 2023

ti-chi-bot bot commented May 24, 2023 •

edited

Loading

codecov bot commented May 25, 2023 •

edited

Loading