resource_manager: add service mode server #5994

lhy1024 · 2023-02-15T10:52:08Z

What problem does this PR solve?

Issue Number: Ref #5837

What is changed and how does it work?

Check List

Tests

Unit test
Integration test
manual test
./bin/pd-server service resource_manager --backend-endpoints="http://127.0.0.1:35207"

Release note

None.

ti-chi-bot · 2023-02-15T10:52:10Z

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

ti-chi-bot · 2023-02-15T10:52:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-02-15T12:23:52Z

Codecov Report

Base: 75.03% // Head: 74.77% // Decreases project coverage by -0.26% ⚠️

Coverage data is based on head (a94eaab) compared to base (31bf4bc).
Patch coverage: 34.82% of modified lines in pull request are covered.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5994      +/-   ##
==========================================
- Coverage   75.03%   74.77%   -0.26%     
==========================================
  Files         363      365       +2     
  Lines       36339    36552     +213     
==========================================
+ Hits        27266    27331      +65     
- Misses       6687     6820     +133     
- Partials     2386     2401      +15

Flag	Coverage Δ
unittests	`74.77% <34.82%> (-0.26%)`	⬇️

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

Impacted Files	Coverage Δ
client/resource_manager_client.go	`66.84% <0.00%> (ø)`
pkg/mcs/resource_manager/server/grpc_service.go	`68.60% <0.00%> (ø)`
pkg/mcs/resource_manager/server/config.go	`1.69% <1.69%> (ø)`
pkg/mcs/resource_manager/server/server.go	`48.05% <48.05%> (ø)`
pkg/mcs/resource_manager/server/manager.go	`79.73% <100.00%> (-0.79%)`	⬇️
server/server.go	`73.99% <100.00%> (-0.23%)`	⬇️
pkg/errs/errs.go	`75.00% <0.00%> (-25.00%)`	⬇️
pkg/dashboard/adapter/manager.go	`71.26% <0.00%> (-8.05%)`	⬇️
server/schedulers/shuffle_hot_region.go	`56.66% <0.00%> (-3.34%)`	⬇️
server/schedule/labeler/rules.go	`87.50% <0.00%> (-2.28%)`	⬇️
... and 13 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.

rleungx · 2023-02-16T02:42:29Z

Could you please add some simple tests to validate it works?

server/server.go

pkg/mcs/resource_manager/server/server.go

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

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

cc @rleungx @binshi-bing

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

pkg/basicserver/basic_server.go

pkg/mcs/resource_manager/server/server.go

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

binshi-bing

left some comments

binshi-bing · 2023-02-17T19:10:47Z

pkg/mcs/resource_manager/server/server.go

+	}
+	// Server has started.
+	atomic.StoreInt64(&s.isServing, 1)
+	return mux.Serve()


if mux.Serve is a blocking call, we might have a bigger issue. It means it won't stay on <-ctx.Done to wait for the exit signals. if a signal is raised, svr.Run() won't return unless network listener is closed but it mightn't.

var sig os.Signal go func() { sig = <-sc cancel() }() if err := svr.Run(); err != nil { log.Fatal("run server failed", errs.ZapError(err)) } <-ctx.Done()

pkg/mcs/resource_manager/server/server.go

binshi-bing · 2023-02-19T22:03:48Z

pkg/mcs/resource_manager/server/server.go

+	}
+
+	s.grpcServer = grpc.NewServer()
+	s.service.RegisterGRPCService(s.grpcServer)


It seems that we should support gPRC Gateway. By default, EnableGRPCGateway is true.

where will we use it?

pkg/mcs/resource_manager/server/server.go

binshi-bing · 2023-02-20T01:48:31Z

pkg/mcs/resource_manager/server/server.go

+	}
+	// Server has started.
+	atomic.StoreInt64(&s.isServing, 1)
+	return mux.Serve()


In this commit 3f050d1, I refactored GRPC and HTTP Server start logic. It refers to DGraph https://github.com/binshi-bing/dgraph/blob/cbec2309413a9b17d407b3417300f719381b36d7/dgraph/cmd/zero/run.go#L318-L319 and cmux example https://github.com/soheilhy/cmux/blob/5ec6847320e53b5fee0ab9a4757b56625a946c85/example/example_test.go#L108.

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

pkg/mcs/resource_manager/server/config.go

rleungx · 2023-02-20T08:52:21Z

pkg/mcs/resource_manager/server/config.go

+	BackendEndpoints  string `toml:"backend-endpoints" json:"backend-endpoints"`
+	ListenAddr        string `toml:"listen-addr" json:"listen-addr"`
+	Name              string `toml:"name" json:"name"`
+	DataDir           string `toml:"data-dir" json:"data-dir"`


What is DataDir used for?

L162 log path?

Log.File.Filename does the same thing?

it will do in another pr

pkg/mcs/resource_manager/server/server.go

rleungx · 2023-02-20T09:10:20Z

pkg/mcs/resource_manager/server/server.go

+	s.service.RegisterGRPCService(gs)
+	err := gs.Serve(l)
+	if err != cmux.ErrListenerClosed {
+		log.Fatal("grpc server stops serving unexpectedly.", errs.ZapError(err))


Suggested change

log.Fatal("grpc server stops serving unexpectedly.", errs.ZapError(err))

log.Fatal("grpc server stops serving unexpectedly", errs.ZapError(err))

Suggest removing all periods in log

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

binshi-bing

LGTM

ti-chi-bot · 2023-02-21T02:41:43Z

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

LGTM

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.

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

binshi-bing

LGTM

ti-chi-bot · 2023-02-21T07:09:16Z

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

LGTM

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.

rleungx

The rest LGTM

pkg/mcs/resource_manager/server/server.go

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

lhy1024 · 2023-02-21T10:42:16Z

/merge

ti-chi-bot · 2023-02-21T10:42:17Z

@lhy1024: 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-21T10:42:20Z

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

Commit hash: a94eaab

ref tikv#5837 Signed-off-by: lhy1024 <admin@liudos.us> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Conflicts: go.mod pkg/mcs/tso/server/server.go tests/client/go.sum tests/mcs/go.sum

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 15, 2023

ti-chi-bot requested review from JmPotato and nolouch February 15, 2023 10:52

lhy1024 force-pushed the rms2 branch from 230d751 to c8c74eb Compare February 15, 2023 11:57

binshi-bing mentioned this pull request Feb 15, 2023

Implement tso grpc service include both server side and client side in a basic manner #5986

Merged

rleungx reviewed Feb 16, 2023

View reviewed changes

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 Feb 16, 2023

resource_manager: add service mode server

0bebe96

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

lhy1024 force-pushed the rms2 branch from 65e0e07 to 0bebe96 Compare February 16, 2023 14:10

lhy1024 marked this pull request as ready for review February 16, 2023 14:10

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2023

lhy1024 requested a review from rleungx February 16, 2023 14:10

lhy1024 added 2 commits February 16, 2023 22:15

fix lint

e12f074

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

Add ReadHeaderTimeout to http.Server

00eced3

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

lhy1024 force-pushed the rms2 branch from 22d66fa to 00eced3 Compare February 16, 2023 14:35

lhy1024 added 2 commits February 16, 2023 22:51

fix lint

f90a988

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

fix test

216b9e4

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

binshi-bing reviewed Feb 16, 2023

View reviewed changes

pkg/basicserver/basic_server.go Outdated Show resolved Hide resolved

binshi-bing reviewed Feb 16, 2023

View reviewed changes

pkg/basicserver/basic_server.go Outdated Show resolved Hide resolved

binshi-bing reviewed Feb 17, 2023

View reviewed changes

pkg/mcs/resource_manager/server/server.go Outdated Show resolved Hide resolved

binshi-bing mentioned this pull request Feb 17, 2023

Separate TSO into an independent service #5836

Closed

lhy1024 added 2 commits February 17, 2023 18:52

rename

f6cefeb

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

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

739ecd7

binshi-bing reviewed Feb 20, 2023

View reviewed changes

lhy1024 added 2 commits February 20, 2023 15:35

update related to tikv#6008

98a6fdb

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

add cmd

5ead2d3

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

rleungx reviewed Feb 20, 2023

View reviewed changes

lhy1024 added 2 commits February 20, 2023 17:44

address comments

4c1911c

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

address comments

b0d7c5f

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

binshi-bing approved these changes Feb 21, 2023

View reviewed changes

nolouch approved these changes Feb 21, 2023

View reviewed changes

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

address comments

27adb0d

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

lhy1024 requested review from rleungx and binshi-bing February 21, 2023 07:03

binshi-bing approved these changes Feb 21, 2023

View reviewed changes

rleungx approved these changes Feb 21, 2023

View reviewed changes

pkg/mcs/resource_manager/server/server.go Outdated Show resolved Hide resolved

pkg/mcs/resource_manager/server/server.go Outdated Show resolved Hide resolved

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 21, 2023

lhy1024 added 2 commits February 21, 2023 17:12

address comments

a9a016a

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

remove todo

a94eaab

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 21, 2023

Merge branch 'master' into rms2

241bb8a

ti-chi-bot merged commit 71f776f into tikv:master Feb 21, 2023

lhy1024 mentioned this pull request Feb 22, 2023

Make the service start with independent mode #5837

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager: add service mode server #5994

resource_manager: add service mode server #5994

lhy1024 commented Feb 15, 2023 •

edited

Loading

ti-chi-bot commented Feb 15, 2023 •

edited

Loading

ti-chi-bot commented Feb 15, 2023

codecov bot commented Feb 15, 2023 •

edited

Loading

rleungx commented Feb 16, 2023

lhy1024 commented Feb 16, 2023

binshi-bing left a comment

binshi-bing Feb 17, 2023

binshi-bing Feb 19, 2023

lhy1024 Feb 20, 2023

binshi-bing Feb 20, 2023

rleungx Feb 20, 2023

lhy1024 Feb 20, 2023

rleungx Feb 20, 2023

lhy1024 Feb 21, 2023

rleungx Feb 20, 2023

rleungx Feb 20, 2023

binshi-bing left a comment

ti-chi-bot commented Feb 21, 2023

binshi-bing left a comment

ti-chi-bot commented Feb 21, 2023

rleungx left a comment

lhy1024 commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

	log.Fatal("grpc server stops serving unexpectedly.", errs.ZapError(err))
	log.Fatal("grpc server stops serving unexpectedly", errs.ZapError(err))

resource_manager: add service mode server #5994

resource_manager: add service mode server #5994

Conversation

lhy1024 commented Feb 15, 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 15, 2023 • edited Loading

ti-chi-bot commented Feb 15, 2023

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

rleungx commented Feb 16, 2023

lhy1024 commented Feb 16, 2023

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

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

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 21, 2023

binshi-bing left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Feb 21, 2023

rleungx left a comment

Choose a reason for hiding this comment

lhy1024 commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

ti-chi-bot commented Feb 21, 2023

lhy1024 commented Feb 15, 2023 •

edited

Loading

ti-chi-bot commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading