mcs: support forwarding requests for tso server #6130

lhy1024 · 2023-03-09T11:58:15Z

What problem does this PR solve?

Issue Number: Ref #5836

What is changed and how does it work?

support forwarding requests for tso server when api server is disable for tso
add switch in api server for tso
add test cluster for api server

Check List

Tests

Unit test

Release note

None.

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

ti-chi-bot · 2023-03-09T11:58:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
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-03-09T11:59:06Z

cc @binshi-bing @rleungx

server/grpc_service.go

tests/mcs/tso/server_test.go

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

codecov · 2023-03-09T13:26:40Z

Codecov Report

Patch coverage: 82.53% and project coverage change: +0.11 🎉

Comparison is base (266c021) 74.31% compared to head (21bfd68) 74.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6130      +/-   ##
==========================================
+ Coverage   74.31%   74.42%   +0.11%     
==========================================
  Files         393      393              
  Lines       38353    38446      +93     
==========================================
+ Hits        28502    28614     +112     
+ Misses       7317     7288      -29     
- Partials     2534     2544      +10

Flag	Coverage Δ
unittests	`74.42% <82.53%> (+0.11%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/mcs/tso/server/server.go	`51.46% <ø> (+0.66%)`	⬆️
pkg/tso/allocator_manager.go	`65.80% <ø> (-0.71%)`	⬇️
pkg/utils/apiutil/serverapi/middleware.go	`63.07% <0.00%> (+1.53%)`	⬆️
server/api/tso.go	`50.00% <ø> (ø)`
server/api/member.go	`53.12% <60.00%> (+0.74%)`	⬆️
server/server.go	`75.76% <74.28%> (+1.55%)`	⬆️
server/grpc_service.go	`51.68% <85.95%> (+2.48%)`	⬆️
tests/mcs/testutil.go	`100.00% <100.00%> (ø)`

... and 26 files with indirect coverage changes

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

binshi-bing

left some comments

pkg/mcs/tso/server/server.go

server/grpc_service.go

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

server/grpc_service.go

rleungx · 2023-03-10T08:52:19Z

server/grpc_service.go

@@ -222,33 +247,38 @@ type tsoRequest struct {
 	stream        pdpb.PD_TsoServer
 }

-func (s *GrpcServer) dispatchTSORequest(ctx context.Context, request *tsoRequest, forwardedHost string, doneCh <-chan struct{}, errCh chan<- error) {
+func (s *GrpcServer) dispatchTSORequest(ctx context.Context, request *tsoRequest, forwardedHost string, doneCh <-chan struct{}, errCh chan<- error, workingMCS bool) {


How about using a clearer name instead of workingMCS?

Do you have a good idea?

The difference inside of dispatchTSORequest is actually about which protocolbuf to use, pdpb or tsopb? How about 'tsoProto bool'. The tso service itself needs to use this framework too.

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

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

lhy1024 · 2023-03-10T14:23:02Z

cc@binshi-bing @rleungx

server/grpc_service.go

rleungx · 2023-03-13T06:53:00Z

server/grpc_service.go

+	done := make(chan struct{})
+	ctx, cancel := context.WithTimeout(s.ctx, defaultTSOProxyTimeout)
+	go checkStream(ctx, cancel, done)
+	forwardStream, err := tsopb.NewTSOClient(client).Tso(ctx)


What if the primary changes?

it will change by

pd/server/grpc_service.go

Line 1965 in 5744ddd

ok, forwardedHost, err := s.GetServicePrimaryAddr(ctx, "tso")

How about the original stream, will it be closed?

original stream? with pdpb proto?

If we create a stream to A, after that, B becomes primary. Then, we create a stream to B. What about A now?

Its behaviour will be similar with other stream in clientConns

Yes, but do we need to consider that?

The current number of streams is directly related to the TSO server, the current number is not much, I will add a TODO first. and do we need to think about the EOF issue, do we keep a TODO too?

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

rleungx · 2023-03-13T07:43:41Z

server/grpc_service.go

+				KeyspaceId:      utils.DefaultKeySpaceGroupID,
+				KeyspaceGroupId: utils.DefaultKeySpaceGroupID,


Do we need them now?

just filled it with default value

I think there is no need to do it for now.

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

rleungx · 2023-03-15T08:31:40Z

server/grpc_service.go

+	done := make(chan struct{})
+	ctx, cancel := context.WithTimeout(s.ctx, defaultTSOProxyTimeout)
+	go checkStream(ctx, cancel, done)
+	forwardStream, err := tsopb.NewTSOClient(client).Tso(ctx)


Why do we need to create a stream here?

I will cache it until forward-host changed

rleungx · 2023-03-15T08:33:44Z

server/grpc_service.go

+				KeyspaceId:      utils.DefaultKeySpaceID,
+				KeyspaceGroupId: utils.DefaultKeySpaceGroupID,


Do we really need these two for now?

only filled it

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

binshi-bing

LGTM

ti-chi-bot · 2023-03-16T04:12:41Z

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

ti-chi-bot · 2023-03-16T05:30:11Z

@lhy1024: PR needs rebase.

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 kubernetes/test-infra repository.

ti-chi-bot · 2023-03-16T05:32:01Z

@lhy1024: PR needs rebase.

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 kubernetes/test-infra repository.

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

tests/mcs/tso/server_test.go

lhy1024 · 2023-03-16T07:51:56Z

/merge

ti-chi-bot · 2023-03-16T07:51:58Z

@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-03-16T07:52:00Z

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

Commit hash: 21bfd68

mcs: support forwarding requests for tso server

9d43bb0

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

ti-chi-bot added the release-note-none label Mar 9, 2023

ti-chi-bot requested review from disksing and JmPotato March 9, 2023 11:58

rleungx reviewed Mar 9, 2023

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

tests/mcs/tso/server_test.go Outdated Show resolved Hide resolved

address comments

769052d

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

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

binshi-bing reviewed Mar 10, 2023

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

binshi-bing reviewed Mar 10, 2023

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

rleungx reviewed Mar 10, 2023

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

server/grpc_service.go Outdated Show resolved Hide resolved

use dispatchTSORequest

087fd63

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

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

lhy1024 added 2 commits March 10, 2023 15:13

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

90ddb1e

update cluster id

7d27776

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

rleungx reviewed Mar 10, 2023

View reviewed changes

server/grpc_service.go Outdated Show resolved Hide resolved

server/grpc_service.go Outdated Show resolved Hide resolved

rleungx reviewed Mar 10, 2023

View reviewed changes

lhy1024 added 6 commits March 10, 2023 18:11

address comments: directly judge by IsAPIServiceMode and add judge to…

c39b836

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

fix conflict

91319a2

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

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

362a41c

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

add test

a6621d2

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

reduce test time

a21150d

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

Merge branch 'master' into forward

5744ddd

rleungx reviewed Mar 13, 2023

View reviewed changes

lhy1024 added 2 commits March 13, 2023 15:31

address comments: rename

5bee178

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

set default keyspace info

90bd7fb

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

rleungx reviewed Mar 13, 2023

View reviewed changes

lhy1024 added 2 commits March 13, 2023 15:46

address comment

ff2c1df

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

use default key space info

8873ed8

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

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

lhy1024 added 2 commits March 14, 2023 14:55

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

a97b730

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

Merge branch 'forward' of github.com:lhy1024/pd into forward

16f560e

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

rleungx reviewed Mar 15, 2023

View reviewed changes

address comments: save tso forward stream

d032ca3

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

rleungx approved these changes Mar 16, 2023

View reviewed changes

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

lhy1024 mentioned this pull request Mar 16, 2023

*: add watch for services' primary address #6155

Merged

binshi-bing approved these changes Mar 16, 2023

View reviewed changes

JmPotato approved these changes Mar 16, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 16, 2023

lhy1024 added 2 commits March 16, 2023 14:23

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

4a8f084

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

add test for disable tso service in api service mode

21bfd68

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

rleungx reviewed Mar 16, 2023

View reviewed changes

tests/mcs/tso/server_test.go Show resolved Hide resolved

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

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

ti-chi-bot merged commit 8ba42df into tikv:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mcs: support forwarding requests for tso server #6130

mcs: support forwarding requests for tso server #6130

lhy1024 commented Mar 9, 2023

ti-chi-bot commented Mar 9, 2023 •

edited

Loading

lhy1024 commented Mar 9, 2023

codecov bot commented Mar 9, 2023 •

edited

Loading

binshi-bing left a comment

rleungx Mar 10, 2023

lhy1024 Mar 10, 2023

binshi-bing Mar 11, 2023

lhy1024 Mar 13, 2023

lhy1024 commented Mar 10, 2023

rleungx Mar 13, 2023

lhy1024 Mar 13, 2023

rleungx Mar 13, 2023

lhy1024 Mar 13, 2023

rleungx Mar 13, 2023

lhy1024 Mar 14, 2023

rleungx Mar 14, 2023

lhy1024 Mar 15, 2023 •

edited

Loading

rleungx Mar 13, 2023

lhy1024 Mar 13, 2023

rleungx Mar 13, 2023

rleungx Mar 15, 2023

lhy1024 Mar 15, 2023

rleungx Mar 15, 2023

lhy1024 Mar 15, 2023

binshi-bing left a comment

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

lhy1024 commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

		KeyspaceId: utils.DefaultKeySpaceGroupID,
		KeyspaceGroupId: utils.DefaultKeySpaceGroupID,

		KeyspaceId: utils.DefaultKeySpaceID,
		KeyspaceGroupId: utils.DefaultKeySpaceGroupID,

mcs: support forwarding requests for tso server #6130

mcs: support forwarding requests for tso server #6130

Conversation

lhy1024 commented Mar 9, 2023

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Mar 9, 2023 • edited Loading

lhy1024 commented Mar 9, 2023

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

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

lhy1024 commented Mar 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

lhy1024 Mar 15, 2023 • edited Loading

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

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

lhy1024 commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 9, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading

lhy1024 Mar 15, 2023 •

edited

Loading