Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

resource_manager: basic implement about manage resource group #5785

Merged
merged 7 commits into from
Dec 22, 2022
Merged

resource_manager: basic implement about manage resource group #5785

merged 7 commits into from
Dec 22, 2022

Conversation

nolouch
Copy link
Contributor

@nolouch nolouch commented Dec 15, 2022

Signed-off-by: nolouch nolouch@gmail.com

What problem does this PR solve?

Issue Number: Ref #5784

What is changed and how does it work?

resource_manager: basic implement about manage resource group

Check List

Tests

  • Unit test
  • Integration test

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 15, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • CabinfeverB
  • JmPotato

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 ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 15, 2022
@nolouch nolouch force-pushed the resource-manager/server_part2 branch from 0a3f5a7 to 822a745 Compare December 16, 2022 08:02
@nolouch nolouch requested review from CabinfeverB and removed request for HunDunDM December 16, 2022 08:13
@codecov
Copy link

codecov bot commented Dec 16, 2022 •
edited
Loading

Codecov Report

Base: 75.54% // Head: 75.58% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (ead68eb) compared to base (420bd35).
Patch coverage: 67.71% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5785      +/-   ##
==========================================
+ Coverage   75.54%   75.58%   +0.03%     
==========================================
  Files         330      337       +7     
  Lines       32996    33278     +282     
==========================================
+ Hits        24927    25153     +226     
- Misses       5911     5957      +46     
- Partials     2158     2168      +10
Flag Coverage Δ
unittests 75.58% <67.71%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
server/storage/storage.go 71.15% <ø> (+3.84%) ⬆️
pkg/mcs/resource_manager/server/token_bukets.go 42.85% <42.85%> (ø)
pkg/mcs/registry/registry.go 81.81% <50.00%> (ø)
pkg/mcs/resource_manager/server/types.go 61.68% <61.68%> (ø)
pkg/mcs/resource_manager/server/manager.go 69.49% <69.49%> (ø)
pkg/mcs/resource_manager/server/grpc_service.go 75.67% <75.67%> (ø)
pkg/mcs/resource_manager/server/apis/v1/api.go 80.00% <80.00%> (ø)
pkg/mcs/resource_manager/server/install/install.go 100.00% <100.00%> (ø)
pkg/storage/endpoint/key_path.go 93.18% <100.00%> (+0.15%) ⬆️
pkg/storage/endpoint/resource_group.go 100.00% <100.00%> (ø)
... and 30 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.

@nolouch nolouch force-pushed the resource-manager/server_part2 branch 2 times, most recently from 01383fa to d0c8b1d Compare December 16, 2022 10:26
@nolouch
resource_manager: basic implement about manage resource group
1b86299 
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch nolouch force-pushed the resource-manager/server_part2 branch from d0c8b1d to 1b86299 Compare December 16, 2022 10:29
JmPotato
JmPotato reviewed Dec 19, 2022
View reviewed changes
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/manager.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/types.go Outdated Show resolved Hide resolved
rleungx
rleungx reviewed Dec 19, 2022
View reviewed changes
pkg/mcs/resource_manager/server/grpc_service.go Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/apis/v1/api.go Outdated Show resolved Hide resolved
pkg/mcs/resource_manager/server/install/install.go Show resolved Hide resolved
pkg/mcs/resource_manager/server/types.go Outdated Show resolved Hide resolved
@nolouch
address comments
400f1ef 
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch nolouch force-pushed the resource-manager/server_part2 branch from e959b99 to 400f1ef Compare December 19, 2022 09:20
@nolouch
update API and add test
8037ade 
Signed-off-by: nolouch <nolouch@gmail.com>
@JmPotato JmPotato self-requested a review December 20, 2022 06:14
@nolouch
address comments
3bb1248 
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch
address comments
a47f944 
Signed-off-by: nolouch <nolouch@gmail.com>
@nolouch
Copy link
Contributor Author

nolouch commented Dec 21, 2022 •
edited
Loading

ptal again, thanks @JmPotato @CabinfeverB @BornChanger

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 22, 2022
@ti-chi-bot
Copy link
Member

@BornChanger: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

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 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 Dec 22, 2022
@nolouch
Copy link
Contributor Author

nolouch commented Dec 22, 2022

/merge

@ti-chi-bot
Copy link
Member

@nolouch: 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
Copy link
Member

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

Commit hash: ead68eb

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 22, 2022
@ti-chi-bot ti-chi-bot merged commit 882eb1d into tikv:master Dec 22, 2022
@nolouch nolouch deleted the resource-manager/server_part2 branch December 22, 2022 07:13
This was referenced Dec 22, 2022
Closed
Closed
sunxiaoguang
sunxiaoguang reviewed Dec 23, 2022
View reviewed changes
pkg/mcs/docs.go
@@ -12,5 +12,5 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// Package mcs used to implement the core logic of the external services which rely on the PD banckend provider.
package mcs
// Package msc used to implement the core logic of the external services which rely on the PD banckend provider.
Copy link
Member

Choose a reason for hiding this comment

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

There is a typo banckend.

sunxiaoguang
sunxiaoguang reviewed Dec 23, 2022
View reviewed changes
pkg/mcs/resource_manager/server/apis/v1/api.go
func (s *Service) RegisterRouter() {
configEndpoint := s.baseEndpoint.Group("/config")
configEndpoint.POST("/group", s.postResourceGroup)
configEndpoint.PUT("group", s.putResourceGroup)
Copy link
Member

Choose a reason for hiding this comment

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

This routing config is probably missing slash in front of group

@CabinfeverB CabinfeverB mentioned this pull request Jan 11, 2023
Closed
7 tasks
binshi-bing
binshi-bing reviewed Feb 9, 2023
View reviewed changes
pkg/mcs/resource_manager/server/manager.go
}

// NewManager returns a new Manager.
func NewManager(srv *server.Server) *Manager {
Copy link
Contributor

Choose a reason for hiding this comment

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

The abbreviation of server is svr instead of srv.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HuSharp HuSharp HuSharp left review comments

@binshi-bing binshi-bing binshi-bing left review comments

@rleungx rleungx rleungx left review comments

@sunxiaoguang sunxiaoguang sunxiaoguang left review comments

@BornChanger BornChanger BornChanger approved these changes

@CabinfeverB CabinfeverB CabinfeverB approved these changes

@JmPotato JmPotato JmPotato approved these changes

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nolouch @ti-chi-bot @JmPotato @sunxiaoguang @CabinfeverB @rleungx @binshi-bing @HuSharp @BornChanger