*: decouple the dependency between server and mcs #5933

lhy1024 · 2023-02-07T14:16:11Z

Signed-off-by: lhy1024 admin@liudos.us

What problem does this PR solve?

Issue Number: Ref #5837

What is changed and how does it work?

move APIServiceGroup to apiutil
replace server with basic_server in mcs
remove init of mcs in main.go

Check List

Tests

Unit test
Integration test

Release note

 None.

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot · 2023-02-07T14:16:13Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

nolouch
rleungx

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.

lhy1024 · 2023-02-07T14:16:41Z

cc @rleungx @binshi-bing

lhy1024 · 2023-02-07T14:21:34Z

server/util.go

-	return dummyServiceRegistry{}
-}
-
-type dummyServiceRegistry struct{}


No longer need dummy

codecov · 2023-02-07T14:23:16Z

Codecov Report

Base: 75.22% // Head: 75.11% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (c80a2a5) compared to base (2be26ff).
Patch coverage: 88.09% of modified lines in pull request are covered.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5933      +/-   ##
==========================================
- Coverage   75.22%   75.11%   -0.11%     
==========================================
  Files         362      362              
  Lines       36221    36232      +11     
==========================================
- Hits        27247    27217      -30     
- Misses       6600     6634      +34     
- Partials     2374     2381       +7

Flag	Coverage Δ
unittests	`75.11% <88.09%> (-0.11%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/autoscaling/service.go	`100.00% <ø> (ø)`
pkg/dashboard/dashboard.go	`90.00% <ø> (ø)`
pkg/mcs/registry/registry.go	`68.00% <ø> (-2.38%)`	⬇️
pkg/mcs/resource_manager/server/manager.go	`85.61% <ø> (ø)`
pkg/swaggerserver/swaggerserver.go	`100.00% <ø> (ø)`
pkg/utils/apiutil/serverapi/middleware.go	`68.25% <ø> (ø)`
server/apiv2/router.go	`100.00% <ø> (ø)`
pkg/mcs/resource_manager/server/grpc_service.go	`68.60% <33.33%> (ø)`
pkg/utils/apiutil/apiutil.go	`69.62% <80.00%> (+1.29%)`	⬆️
pkg/mcs/resource_manager/server/apis/v1/api.go	`80.00% <100.00%> (ø)`
... and 29 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: lhy1024 <admin@liudos.us>

rleungx · 2023-02-08T08:04:40Z

pkg/mcs/registry/registry.go

@@ -22,7 +21,7 @@ import (
 	"net/http"

 	"github.com/pingcap/log"
-	"github.com/tikv/pd/server"
+	bs "github.com/tikv/pd/pkg/basic_server"


Suggested change

bs "github.com/tikv/pd/pkg/basic_server"

bs "github.com/tikv/pd/pkg/basicserver"

how about basicsvr?

I also think basicserver is better than basicsvr

binshi-bing

reverted invalid comment

ti-chi-bot · 2023-02-08T18:12:13Z

@binshi-bing: Request changes is only allowed for the reviewers in list.

In response to this:

This pr seems to split from #5858. Regarding this pr, I have the same ask in #5858 and pasted below:

@lhy1024 , could you please split the common part (e.g., move APIServiceGroup from server to pkg/utils/apiutil, registry refactor) required by other services, into a separate pr and check in first. The common part in this pr, including the way to parse subcommand as discussed, and tso mcs have higher priority than resource manager mcs. In this way, we can both work on tso mcs and resource manager mcs more efficiently.

cc @rleungx

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.

binshi-bing

This pr seems to split from #5858. Regarding this pr, I have the same ask in #5858 and pasted below:

@lhy1024 , thanks for spliting the common part (e.g., move APIServiceGroup from server to pkg/utils/apiutil, registry refactor) required by other services, from #5833 into this separate pr. They have higher priority than the remaining part in the original pr (mainly resource manager mcs related). Could you merge this one earlier so that we can both work on tso mcs and resource manager mcs more efficiently.

ti-chi-bot · 2023-02-08T18:37:18Z

@binshi-bing: 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:

This pr seems to split from #5858. Regarding this pr, I have the same ask in #5858 and pasted below:

@lhy1024 , thanks for spliting the common part (e.g., move APIServiceGroup from server to pkg/utils/apiutil, registry refactor) required by other services, from #5833 into this separate pr. They have higher priority than the remaining part in the original pr (mainly resource manager mcs related). Could you merge this one earlier so that we can both work on tso mcs and resource manager mcs more efficiently.

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.

pkg/basic_server/basic_server.go

binshi-bing · 2023-02-08T18:46:11Z

pkg/basic_server/basic_server.go

+	// GetMember returns the member information.
+	GetMember() *member.Member
+	// AddLeaderCallback adds a callback in the leader campaign phase.
+	AddLeaderCallback(callbacks ...func(context.Context))


Do we need to put this API into basicserver.Server? This API means that this server will elect for leader, correct? It doesn't sound a must-havior behavior of a basic server.

I also consider to replace it with primary

lhy1024 · 2023-02-09T03:43:27Z

This pr seems to split from #5858. Regarding this pr, I have the same ask in #5858 and pasted below:

@lhy1024 , thanks for spliting the common part (e.g., move APIServiceGroup from server to pkg/utils/apiutil, registry refactor) required by other services, from #5833 into this separate pr. They have higher priority than the remaining part in the original pr (mainly resource manager mcs related). Could you merge this one earlier so that we can both work on tso mcs and resource manager mcs more efficiently.

In master branch, resource management is imported by install, which means it will always start however service mode. So we need to change it for tso mode.

Signed-off-by: lhy1024 <admin@liudos.us> Conflicts: cmd/pd-server/main.go pkg/mcs/tso/server/server.go server/server.go

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx · 2023-02-10T06:08:48Z

Please change the file back to 100644

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp · 2023-02-10T08:31:53Z

pkg/mcs/registry/registry.go

@@ -97,9 +97,3 @@ func (r *ServiceRegistry) InstallAllRESTHandler(srv *server.Server, h map[string
 func (r ServiceRegistry) RegisterService(name string, service ServiceBuilder) {


How about turn RegisterService func to pointer receivers
func (r *ServiceRegistry) RegisterService?

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp · 2023-02-10T09:34:03Z

server/testutil.go

@@ -137,3 +140,21 @@ func MustWaitLeader(re *require.Assertions, svrs []*Server) *Server {
 	})
 	return leader
 }
+
+// CreateMokHandler creates a mock handler for test.
+func CreateMokHandler(re *require.Assertions, ip string) HandlerBuilder {


I'm not sure why it was called mok before, is mock written wrong?

server_test.go and func CreateMokHandler also has some legacy mok

ptal @lhy1024

replace name in url

Signed-off-by: lhy1024 <admin@liudos.us>

disksing · 2023-02-10T11:14:21Z

/merge

ti-chi-bot · 2023-02-10T11:14:24Z

@disksing: 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-02-10T11:14:26Z

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

Commit hash: f526604

ref tikv#5837 Signed-off-by: lhy1024 <admin@liudos.us>

*: decouple the dependency between server and mcs

09728d3

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 requested review from nolouch and rleungx February 7, 2023 14:16

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

lhy1024 commented Feb 7, 2023

View reviewed changes

server/util.go

return dummyServiceRegistry{}

}

type dummyServiceRegistry struct{}

Copy link

Contributor Author

lhy1024 Feb 7, 2023

Choose a reason for hiding this comment

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

No longer need dummy

lhy1024 mentioned this pull request Feb 7, 2023

Make the service start with independent mode #5837

Closed

12 tasks

lhy1024 added the type/refactor The issue belongs to a refactor work. label Feb 7, 2023

rm install

7fca2da

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the decouple branch from 06de102 to 7fca2da Compare February 7, 2023 15:55

binshi-bing mentioned this pull request Feb 7, 2023

Separate TSO into an independent service #5836

Closed

rleungx reviewed Feb 8, 2023

View reviewed changes

binshi-bing requested changes Feb 8, 2023

View reviewed changes

binshi-bing approved these changes Feb 8, 2023

View reviewed changes

binshi-bing reviewed Feb 8, 2023

View reviewed changes

pkg/basic_server/basic_server.go Show resolved Hide resolved

binshi-bing reviewed Feb 8, 2023

View reviewed changes

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

lhy1024 added 3 commits February 9, 2023 19:52

address comments

1d115f0

Signed-off-by: lhy1024 <admin@liudos.us> Conflicts: cmd/pd-server/main.go pkg/mcs/tso/server/server.go server/server.go

Merge branch 'master' of http://github.com/tikv/pd into decouple

ae4c79c

Signed-off-by: lhy1024 <admin@liudos.us> Conflicts: cmd/pd-server/main.go pkg/mcs/tso/server/server.go server/server.go

fix conflict

4308fa2

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2023

lhy1024 added 3 commits February 9, 2023 23:45

fix handler nil

e9b8242

Signed-off-by: lhy1024 <admin@liudos.us>

add failpoint

268b24d

Signed-off-by: lhy1024 <admin@liudos.us>

use mok

8e199be

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 added 4 commits February 10, 2023 14:25

address comments

d33dd37

Signed-off-by: lhy1024 <admin@liudos.us>

move test

69d0b1a

Signed-off-by: lhy1024 <admin@liudos.us>

fix client test

7f17cb9

Signed-off-by: lhy1024 <admin@liudos.us>

go mod tidy

6b9824e

Signed-off-by: lhy1024 <admin@liudos.us>

rleungx approved these changes Feb 10, 2023

View reviewed changes

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

HuSharp reviewed Feb 10, 2023

View reviewed changes

address comments

730b9d0

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp reviewed Feb 10, 2023

View reviewed changes

address comments: rename

c80a2a5

Signed-off-by: lhy1024 <admin@liudos.us>

nolouch approved these changes Feb 10, 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 Feb 10, 2023

address comments: fix url

f526604

Signed-off-by: lhy1024 <admin@liudos.us>

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

ti-chi-bot merged commit 60e4594 into tikv:master Feb 10, 2023

lhy1024 deleted the decouple branch February 10, 2023 12:06

nolouch pushed a commit to nolouch/pd that referenced this pull request Feb 24, 2023

*: decouple the dependency between server and mcs (tikv#5933)

75673ae

ref tikv#5837 Signed-off-by: lhy1024 <admin@liudos.us>

rleungx mentioned this pull request Mar 2, 2023

Decoupling the resource manager service relies on pd/server package #5804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: decouple the dependency between server and mcs #5933

*: decouple the dependency between server and mcs #5933

lhy1024 commented Feb 7, 2023 •

edited

Loading

ti-chi-bot commented Feb 7, 2023 •

edited

Loading

lhy1024 commented Feb 7, 2023

lhy1024 Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

rleungx Feb 8, 2023

binshi-bing Feb 8, 2023

lhy1024 Feb 9, 2023

binshi-bing left a comment •

edited

Loading

ti-chi-bot commented Feb 8, 2023

binshi-bing left a comment

ti-chi-bot commented Feb 8, 2023

binshi-bing Feb 8, 2023

lhy1024 Feb 9, 2023

lhy1024 Feb 9, 2023

lhy1024 commented Feb 9, 2023

rleungx commented Feb 10, 2023

HuSharp Feb 10, 2023

lhy1024 Feb 10, 2023

HuSharp Feb 10, 2023

lhy1024 Feb 10, 2023

HuSharp Feb 10, 2023

nolouch Feb 10, 2023

lhy1024 Feb 10, 2023

disksing commented Feb 10, 2023

ti-chi-bot commented Feb 10, 2023

ti-chi-bot commented Feb 10, 2023

	bs "github.com/tikv/pd/pkg/basic_server"
	bs "github.com/tikv/pd/pkg/basicserver"

		@@ -97,9 +97,3 @@ func (r ServiceRegistry) InstallAllRESTHandler(srv server.Server, h map[string
		func (r ServiceRegistry) RegisterService(name string, service ServiceBuilder) {

*: decouple the dependency between server and mcs #5933

*: decouple the dependency between server and mcs #5933

Conversation

lhy1024 commented Feb 7, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 7, 2023 • edited Loading

lhy1024 commented Feb 7, 2023

Choose a reason for hiding this comment

codecov bot commented Feb 7, 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

binshi-bing left a comment • edited Loading

Choose a reason for hiding this comment

ti-chi-bot commented Feb 8, 2023

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhy1024 commented Feb 9, 2023

rleungx commented Feb 10, 2023

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

Choose a reason for hiding this comment

disksing commented Feb 10, 2023

ti-chi-bot commented Feb 10, 2023

ti-chi-bot commented Feb 10, 2023

lhy1024 commented Feb 7, 2023 •

edited

Loading

ti-chi-bot commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 7, 2023 •

edited

Loading

binshi-bing left a comment •

edited

Loading