client, resource_manager: resource manager client by CabinfeverB · Pull Request #5810 · tikv/pd

CabinfeverB · 2022-12-29T12:26:19Z

What problem does this PR solve?

Issue Number: Ref #5851
impl resource manager client

What is changed and how does it work?

impl resource manager client

Check List

Tests

Unit test
Integration test

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 · 2022-12-29T12:26:21Z

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

Details

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

codecov · 2022-12-29T12:35:01Z

Codecov Report

Base: 75.35% // Head: 74.91% // Decreases project coverage by -0.43% ⚠️

Coverage data is based on head (00d13b6) compared to base (a1317a7).
Patch coverage: 25.18% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5810      +/-   ##
==========================================
- Coverage   75.35%   74.91%   -0.44%     
==========================================
  Files         338      337       -1     
  Lines       33858    33941      +83     
==========================================
- Hits        25514    25428      -86     
- Misses       6137     6329     +192     
+ Partials     2207     2184      -23

Flag	Coverage Δ
unittests	`74.91% <25.18%> (-0.44%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/mcs/resource_manager/server/token_bukets.go	`89.18% <ø> (ø)`
client/resourcemanager_client.go	`24.62% <24.62%> (ø)`
client/client.go	`67.59% <100.00%> (+0.13%)`	⬆️
pkg/storage/endpoint/resource_group.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/mcs/resource_manager/server/manager.go	`0.00% <0.00%> (-69.50%)`	⬇️
pkg/mcs/resource_manager/server/grpc_service.go	`0.00% <0.00%> (-40.58%)`	⬇️
pkg/mcs/resource_manager/server/resource_group.go	`25.23% <0.00%> (-34.58%)`	⬇️
pkg/mcs/registry/registry.go	`63.63% <0.00%> (-18.19%)`	⬇️
pkg/dashboard/adapter/manager.go	`78.16% <0.00%> (-3.45%)`	⬇️
server/schedulers/random_merge.go	`62.50% <0.00%> (-3.13%)`	⬇️
... and 28 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> impl token request and fix update bug Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

HuSharp · 2023-01-04T10:06:33Z

client/resoucemanager_client.go

@@ -0,0 +1,265 @@
+package pd


file name need to be resource rather than resouce

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

client/resoucemanager_client.go

JmPotato · 2023-01-06T09:09:04Z

client/resoucemanager_client.go

+// ListResourceGroups loads and returns target keyspace's metadata.
+func (c *client) ListResourceGroups(ctx context.Context) ([]*rmpb.ResourceGroup, error) {
+	req := &rmpb.ListResourceGroupsRequest{}
+	resp, err := c.resouceManagerClient().ListResourceGroups(ctx, req)


Won't this cause panic if the c.resouceManagerClient() return nil?

Like leaderClient, I don't think c.resouceManagerClient() is going to return nil

client/resoucemanager_client.go

JmPotato · 2023-01-06T09:12:38Z

client/resoucemanager_client.go

+}
+
+func (c *client) AddResourceGroup(ctx context.Context, resourceGroupName string, settings *rmpb.GroupSettings) (string, error) {
+	return c.putResourceGroup(ctx, resourceGroupName, settings, 0 /* type add resource group */)


What about using an enum type to distinguish the type rather than the comment?

client/resoucemanager_client.go

JmPotato · 2023-01-06T09:16:02Z

client/resoucemanager_client.go

+		clientCtx:  c.ctx,
+	}
+	req.Requeset = request
+	c.tokenDispatcher.tokenBatchController.tokenRequestCh <- req


It seems that this will easily be blocked since the tokenRequestCh's capacity is 1.

Currently we have not implemented batch, so it will also be blocked by Wait()？

JmPotato · 2023-01-06T09:16:41Z

client/resoucemanager_client.go

+	dispatcher := &tokenDispatcher{
+		dispatcherCancel: dispatcherCancel,
+		tokenBatchController: newTokenBatchController(
+			make(chan *tokenRequest, 1)),


What about using a larger size?

Currently we have not implemented batch and do not seem to need a larger size

client/resoucemanager_client.go

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

…ient

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

JmPotato · 2023-01-09T03:36:53Z

pkg/mcs/resource_manager/server/token_bukets.go

 const defaultInitialTokens = 10 * 10000

+const defaultMaxTokens = 1e7


Suggested change

const defaultInitialTokens = 10 * 10000

const defaultMaxTokens = 1e7

const (

defaultInitialTokens = 10 * 10000

defaultMaxTokens = 1e7

)

JmPotato · 2023-01-09T03:39:16Z

pkg/mcs/resource_manager/server/token_bukets.go

 		t.Tokens += float64(t.Settings.FillRate) * delta.Seconds()
 		t.LastUpdate = &now
 	}
+	if t.Tokens > t.MaxTokens {


Do we also need to check the max tokens when it has not been initialized below?

JmPotato · 2023-01-09T03:40:25Z

pkg/mcs/resource_manager/server/token_bukets.go

+	var res rmpb.TokenBucket
+	res.Settings = &rmpb.TokenLimitSettings{}
+	// FillRate is used for the token server unavailable in abnormal situation.
+	res.Settings.FillRate = 0


I think this initialization is unnecessary.

JmPotato · 2023-01-09T03:41:03Z

pkg/mcs/resource_manager/server/token_bukets.go

+	var trickleTime = time.Duration(targetPeriodMs) * time.Millisecond
+	availableRate := float64(t.Settings.FillRate)
+	if debt := -t.Tokens; debt > 0 {
+		fmt.Println(debt)


Suggested change

fmt.Println(debt)

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

…ient

HuSharp · 2023-01-09T09:12:34Z

client/resourcemanager_client.go

+	AcquireTokenBuckets(ctx context.Context, request *rmpb.TokenBucketsRequest) ([]*rmpb.TokenBucketResponse, error)
+}
+
+// resouceManagerClient gets the ResourceManager client of current PD leader.


I'm resouce killer!!!🤣can found 10 resouce by Ctrl-F

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

HuSharp · 2023-01-10T07:53:26Z

client/resourcemanager_client.go

+	return nil
+}
+
+// ListResourceGroups loads and returns target keyspace's metadata.


resource groups

JmPotato

The rest LGTM.

JmPotato · 2023-01-10T09:42:58Z

client/resourcemanager_client.go

+	return nil
+}
+
+// ListResourceGroups loads and returns target keyspace's metadata.


The comment is incorrect.

Suggested change

// ListResourceGroups loads and returns target keyspace's metadata.

// ListResourceGroups loads and returns xxx.

JmPotato · 2023-01-10T09:46:58Z

client/resourcemanager_client.go

+	return nil
+}
+
+func (c *client) finishTokenRequest(t *tokenRequest, tokenBuckets []*rmpb.TokenBucketResponse, err error) {


It seems that this function is too simple to exist. Maybe we can just remove it to avoid some data copying.

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

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

nolouch · 2023-01-11T04:16:23Z

client/resourcemanager_client.go

+)
+
+// ResourceManagerClient manages resource group info and token request.
+type ResourceManagerClient interface {


you can replace the client in tests/msc/resource_manager_test.go to add the tests.

JmPotato

LGTM

client/client.go

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

nolouch

lgtm

CabinfeverB · 2023-01-11T09:22:26Z

/merge

ti-chi-bot · 2023-01-11T09:22:27Z

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

/run-all-tests

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 · 2023-01-11T09:22:29Z

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

Details

Commit hash: d776fca

ti-chi-bot · 2023-01-11T09:22:42Z

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

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.

CabinfeverB added 2 commits December 29, 2022 20:12

impl token request

ca83b1d

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

impl resouce manager client

497c67d

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

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 29, 2022

ti-chi-bot requested review from JmPotato and nolouch December 29, 2022 12:26

CabinfeverB added 2 commits January 3, 2023 17:29

impl token request and fix update bug

321c2ad

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

add test

0fc2ca5

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> impl token request and fix update bug Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

HuSharp reviewed Jan 4, 2023

View reviewed changes

client/resoucemanager_client.go

@@ -0,0 +1,265 @@

package pd

Copy link

Member

HuSharp Jan 4, 2023

Choose a reason for hiding this comment

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

file name need to be resource rather than resouce

CabinfeverB added 3 commits January 5, 2023 16:48

address comment

ef25725

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

address comment

6a13cab

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

address comment

6f18863

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

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

JmPotato reviewed Jan 6, 2023

View reviewed changes

CabinfeverB added 4 commits January 6, 2023 17:18

merge master

2d2fc55

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

merge master

887d221

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

Merge branch 'master' into resource-manager/client

1b769a6

Merge branch 'resource-manager/server_part3' into resource-manager/cl…

356f118

…ient

nolouch mentioned this pull request Jan 6, 2023

Global Resource Quota Control pingcap/tidb#38825

Closed

41 tasks

address comment

7bc3bc9

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

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

JmPotato reviewed Jan 9, 2023

View reviewed changes

CabinfeverB added 3 commits January 9, 2023 14:28

address comment

f73f782

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

address comment

d5aa646

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

Merge branch 'resource-manager/server_part3' into resource-manager/cl…

72079f2

…ient

HuSharp reviewed Jan 9, 2023

View reviewed changes

address comment

29f44e1

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

HuSharp reviewed Jan 10, 2023

View reviewed changes

JmPotato reviewed Jan 10, 2023

View reviewed changes

CabinfeverB added 3 commits January 10, 2023 18:15

address comment

eefe996

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

merge master

69a3ac0

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

merge master

6115bc6

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

CabinfeverB force-pushed the resource-manager/client branch from 62b275c to 6115bc6 Compare January 11, 2023 03:57

nolouch reviewed Jan 11, 2023

View reviewed changes

JmPotato approved these changes Jan 11, 2023

View reviewed changes

client/client.go Outdated Show resolved Hide resolved

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

add test

d776fca

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

CabinfeverB mentioned this pull request Jan 11, 2023

Tracking issue for resource manager #5851

Closed

7 tasks

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 11, 2023

nolouch approved these changes Jan 11, 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 Jan 11, 2023

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

Merge branch 'master' into resource-manager/client

00d13b6

ti-chi-bot merged commit 59a6203 into tikv:master Jan 11, 2023

rleungx mentioned this pull request Jan 12, 2023

*: add Makefile for msc #5856

Merged

		const defaultInitialTokens = 10 * 10000

		const defaultMaxTokens = 1e7

	// ListResourceGroups loads and returns target keyspace's metadata.
	// ListResourceGroups loads and returns xxx.

Conversation

CabinfeverB commented Dec 29, 2022 • 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 does it work?

Check List

Release note

Uh oh!

ti-chi-bot commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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

JmPotato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

CabinfeverB commented Jan 11, 2023

Uh oh!

ti-chi-bot commented Jan 11, 2023

Uh oh!

ti-chi-bot commented Jan 11, 2023

Uh oh!

ti-chi-bot commented Jan 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

CabinfeverB commented Dec 29, 2022 •

edited

Loading

ti-chi-bot commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading