resourcemanager: do not check existence when add resource group #6717

glorv · 2023-06-29T09:26:46Z

What problem does this PR solve?

Issue Number: ref #5851, ref pingcap/tidb#45050

What is changed and how does it work?

Do not check the existence of target resource group to make create resource group idempotent.

Check List

Tests

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

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.

ti-chi-bot · 2023-06-29T09:26:48Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

Signed-off-by: glorv <glorvs@163.com>

codecov · 2023-06-30T03:31:17Z

Codecov Report

Patch coverage has no change and project coverage change: +0.32 🎉

Comparison is base (87b5ade) 74.30% compared to head (4b9be9c) 74.63%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6717      +/-   ##
==========================================
+ Coverage   74.30%   74.63%   +0.32%     
==========================================
  Files         410      417       +7     
  Lines       42853    43625     +772     
==========================================
+ Hits        31842    32558     +716     
- Misses       8158     8181      +23     
- Partials     2853     2886      +33

Flag	Coverage Δ
unittests	`74.63% <ø> (+0.32%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/mcs/resourcemanager/server/manager.go	`82.10% <ø> (+0.56%)`	⬆️

... and 43 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.

HuSharp · 2023-06-30T06:38:59Z

tests/integrations/mcs/resourcemanager/resource_manager_test.go

@@ -789,7 +789,7 @@ func (suite *resourceManagerClientTestSuite) TestBasicResourceGroupCURD() {
 		}
 		// Create Resource Group
 		resp, err := cli.AddResourceGroup(suite.ctx, group)
-		checkErr(err, tcase.addSuccess)
+		checkErr(err, true)
 		if tcase.addSuccess {


Maybe it's better to change addSuccess name to meet now the test AddResourceGroup behavior will actually succeed.

Signed-off-by: glorv <glorvs@163.com>

HuSharp

LGTM!

CabinfeverB

Can we add some comment for this behavior? REST LGTM

HuSharp · 2023-06-30T08:40:37Z

pkg/mcs/resourcemanager/server/manager.go

-	_, ok := m.groups[grouppb.Name]
-	m.RUnlock()
-	if ok {
-		return errs.ErrResourceGroupAlreadyExists.FastGenByArgs(grouppb.Name)


Need to remove the ErrResourceGroupAlreadyExists related error definition.

Signed-off-by: glorv <glorvs@163.com>

CabinfeverB · 2023-07-03T06:43:34Z

/merge

ti-chi-bot · 2023-07-03T06:43:35Z

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

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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-07-03T06:43:36Z

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

Commit hash: 4b9be9c

ti-chi-bot · 2023-07-03T06:53:51Z

In response to a cherrypick label: new pull request created to branch release-7.1: #6739.

… (#6739) ref #5851, ref pingcap/tidb#45050 Signed-off-by: glorv <glorvs@163.com> Co-authored-by: glorv <glorvs@163.com> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added release-note-none needs-cherry-pick-release-7.1 labels Jun 29, 2023

ti-chi-bot bot requested review from lhy1024 and nolouch June 29, 2023 09:26

do not check existance when add resource group

80fa75e

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 29, 2023

glorv force-pushed the add-rg branch from 7db5d8e to 80fa75e Compare June 29, 2023 09:27

fix test

1e51a60

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 29, 2023

glorv added 2 commits June 30, 2023 11:20

fix test

73c2bb6

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'master' into add-rg

c0a96e9

HuSharp reviewed Jun 30, 2023

View reviewed changes

glorv added 2 commits June 30, 2023 14:58

change name

6e5a1bc

Signed-off-by: glorv <glorvs@163.com>

Merge branch 'master' into add-rg

2c21402

HuSharp approved these changes Jun 30, 2023

View reviewed changes

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

HuSharp requested review from CabinfeverB and removed request for lhy1024 June 30, 2023 07:04

CabinfeverB approved these changes Jun 30, 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 Jun 30, 2023

HuSharp reviewed Jun 30, 2023

View reviewed changes

remove useless error code

0c1d3be

Signed-off-by: glorv <glorvs@163.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 3, 2023

glorv added 2 commits July 3, 2023 10:53

Merge branch 'master' into add-rg

74cc889

fix typo

4b9be9c

Signed-off-by: glorv <glorvs@163.com>

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

Merge branch 'master' into add-rg

a6de32e

ti-chi-bot bot merged commit 5f7236f into tikv:master Jul 3, 2023
22 checks passed

ti-chi-bot mentioned this pull request Jul 3, 2023

resourcemanager: do not check existence when add resource group (#6717) #6739

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resourcemanager: do not check existence when add resource group #6717

resourcemanager: do not check existence when add resource group #6717

glorv commented Jun 29, 2023

ti-chi-bot bot commented Jun 29, 2023 •

edited

codecov bot commented Jun 30, 2023 •

edited

HuSharp Jun 30, 2023

glorv Jun 30, 2023

HuSharp left a comment

CabinfeverB left a comment

HuSharp Jun 30, 2023

glorv Jul 3, 2023

CabinfeverB commented Jul 3, 2023

ti-chi-bot bot commented Jul 3, 2023

ti-chi-bot bot commented Jul 3, 2023

ti-chi-bot commented Jul 3, 2023

resourcemanager: do not check existence when add resource group #6717

resourcemanager: do not check existence when add resource group #6717

Conversation

glorv commented Jun 29, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Jun 29, 2023 • edited

codecov bot commented Jun 30, 2023 • edited

Codecov Report

HuSharp Jun 30, 2023

Choose a reason for hiding this comment

glorv Jun 30, 2023

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

CabinfeverB left a comment

Choose a reason for hiding this comment

HuSharp Jun 30, 2023

Choose a reason for hiding this comment

glorv Jul 3, 2023

Choose a reason for hiding this comment

CabinfeverB commented Jul 3, 2023

ti-chi-bot bot commented Jul 3, 2023

ti-chi-bot bot commented Jul 3, 2023

ti-chi-bot commented Jul 3, 2023

ti-chi-bot bot commented Jun 29, 2023 •

edited

codecov bot commented Jun 30, 2023 •

edited