*: support dynamic tso service #8517

rleungx · 2024-08-12T08:09:05Z

What problem does this PR solve?

Issue Number: ref #8477

What is changed and how does it work?

Check List

Tests

Unit test

Release note

None.

JmPotato · 2024-08-15T05:32:38Z

client/client.go

@@ -660,7 +660,7 @@ func (c *client) Close() {
 	}
 }

-func (c *client) setServiceMode(newMode pdpb.ServiceMode) {
+func (c *client) setServiceMode(newMode pdpb.ServiceMode, skipSameMode bool) {


Prefer using a more straightforward word.

Suggested change

func (c *client) setServiceMode(newMode pdpb.ServiceMode, skipSameMode bool) {

func (c *client) setServiceMode(newMode pdpb.ServiceMode, force bool) {

It's not the same as force.

why not skipSameMode always?

JmPotato · 2024-08-15T05:34:50Z

client/errs/errs.go

+	}
+	errMsg := err.Error()
+	return strings.Contains(errMsg, "not found tso address") ||
+		strings.Contains(errMsg, "maximum number of retries exceeded")


This error would also occur when the leadership cannot be elected. In which case will this be a misjudgment?

We don't check this error on client side, do you know about the reason?

JmPotato · 2024-08-15T05:37:00Z

tests/integrations/client/client_test.go

@@ -406,6 +406,8 @@ func TestTSOFollowerProxyWithTSOService(t *testing.T) {
 	backendEndpoints := pdLeaderServer.GetAddr()
 	tsoCluster, err := tests.NewTestTSOCluster(ctx, 2, backendEndpoints)
 	re.NoError(err)
+	// let service discovery know the TSO service
+	time.Sleep(500 * time.Millisecond)


Can it be replaced with an Eventually?

codecov · 2024-08-21T06:44:34Z

Codecov Report

Attention: Patch coverage is 76.43678% with 41 lines in your changes missing coverage. Please review.

Project coverage is 71.80%. Comparing base (26ced22) to head (edf3908).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8517      +/-   ##
==========================================
- Coverage   77.54%   71.80%   -5.74%     
==========================================
  Files         474      517      +43     
  Lines       62359    67531    +5172     
==========================================
+ Hits        48358    48493     +135     
- Misses      10441    15482    +5041     
+ Partials     3560     3556       -4

Flag	Coverage Δ
unittests	`71.80% <76.43%> (-5.74%)`	⬇️

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

okJiang · 2024-08-29T06:40:09Z

client/errs/errs.go

@@ -38,6 +38,16 @@ func IsLeaderChange(err error) bool {
 		strings.Contains(errMsg, NotPrimaryErr)
 }

+// IsServiceModeChange will determine whether there is a service mode change.


Suggested change

// IsServiceModeChange will determine whether there is a service mode change.

// IsServiceModeChange determines whether there is a service mode change.

okJiang · 2024-08-29T06:46:26Z

server/forward.go

-		if err != nil {
-			if needRetry := handleStreamError(err); needRetry {
-				continue
+	if s.forwardToTSOService() {


We can reduce some of the indentation

Suggested change

if s.forwardToTSOService() {

if !s.forwardToTSOService() {

return s.tsoAllocatorManager.HandleRequest(ctx, tso.GlobalDCLocation, 1)

}

request := xxxx

.....

okJiang · 2024-08-29T07:51:22Z

server/grpc_service.go

@@ -569,6 +590,72 @@ func (s *GrpcServer) Tso(stream pdpb.PD_TsoServer) error {
 			continue
 		}

+		if s.forwardToTSOService() {


maybe wrap three situation(local tso/ms tso/normal) to three functions. Just a suggestion.

okJiang · 2024-08-29T08:03:10Z

server/cluster/cluster.go

+			if !c.IsServiceIndependent(constant.TSOServiceName) {
+				// leader tso service exit, tso independent service provide tso
+				c.tsoAllocator.ResetAllocatorGroup(tso.GlobalDCLocation, true)
+			}
+			if !c.IsServiceIndependent(constant.TSOServiceName) {
+				log.Info("TSO server starts to provide timestamp")
+			}
+			c.SetServiceIndependent(constant.TSOServiceName)


Suggested change

if !c.IsServiceIndependent(constant.TSOServiceName) {

// leader tso service exit, tso independent service provide tso

c.tsoAllocator.ResetAllocatorGroup(tso.GlobalDCLocation, true)

}

if !c.IsServiceIndependent(constant.TSOServiceName) {

log.Info("TSO server starts to provide timestamp")

}

c.SetServiceIndependent(constant.TSOServiceName)

if !c.IsServiceIndependent(constant.TSOServiceName) {

// leader tso service exit, tso independent service provide tso

c.tsoAllocator.ResetAllocatorGroup(tso.GlobalDCLocation, true)

log.Info("TSO server starts to provide timestamp")

}

c.SetServiceIndependent(constant.TSOServiceName)

okJiang · 2024-08-29T08:15:15Z

server/cluster/cluster.go

@@ -390,24 +397,84 @@ func (c *RaftCluster) checkServices() {
 	}
 }

+// checkTSOService checks the TSO service.
+func (c *RaftCluster) checkTSOService() {


can you add more comments for this function and inside this function. Seems there are too many situations in this function

okJiang · 2024-08-29T08:17:58Z

client/pd_service_discovery.go


 	ctx, cancel := context.WithCancel(c.ctx)
 	defer cancel()
 	ticker := time.NewTicker(serviceModeUpdateInterval)
+	failpoint.Inject("fastUpdateServiceMode", func() {
+		ticker.Stop()
+		ticker = time.NewTicker(10 * time.Millisecond)


ticker.Reset()?

okJiang · 2024-08-29T08:27:57Z

client/client.go

@@ -713,6 +712,7 @@ func (c *client) resetTSOClientLocked(mode pdpb.ServiceMode) {
 		log.Warn("[pd] intend to switch to unknown service mode, just return")
 		return
 	}
+	// Replace the old TSO client.


okJiang · 2024-08-29T08:30:09Z

client/client.go

@@ -660,7 +660,7 @@ func (c *client) Close() {
 	}
 }

-func (c *client) setServiceMode(newMode pdpb.ServiceMode) {
+func (c *client) setServiceMode(newMode pdpb.ServiceMode, skipSameMode bool) {


why not skipSameMode always?

ti-chi-bot · 2024-09-06T08:36:06Z

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-sigs/prow repository.

Signed-off-by: Ryan Leung <[email protected]>

okJiang · 2024-10-08T10:02:30Z

tests/integrations/mcs/tso/server_test.go

-
-func (suite *APIServerForward) checkAvailableTSO(re *require.Assertions) {
-	mcs.WaitForTSOServiceAvailable(suite.ctx, re, suite.pdClient)
+func (suite *APIServerForward) checkAvailableTSO(re *require.Assertions, needWait bool) {


Prefer to wrap to two functions:

checkAvailableTSO

waitAvailableTSO

Signed-off-by: Ryan Leung <[email protected]>

okJiang · 2024-10-09T03:44:34Z

client/errs/errs.go

+		return false
+	}
+	errMsg := err.Error()
+	return strings.Contains(errMsg, "not found tso address") ||


Can we have a method to use ErrorCode or error type to instead of strings.Contains here?

use a constant as IsLeaderChange

okJiang · 2024-10-09T06:53:38Z

server/cluster/cluster.go

+	if c.IsServiceIndependent(constant.TSOServiceName) {
+		log.Info("PD server starts to provide timestamp")
+	}
+	c.UnsetServiceIndependent(constant.TSOServiceName)


Why Unset the flag whatever it is tso service or pd server.

Maybe we can add more comments for independentServices sync.Map

okJiang · 2024-10-09T07:21:54Z

server/grpc_service.go

@@ -569,6 +590,72 @@ func (s *GrpcServer) Tso(stream pdpb.PD_TsoServer) error {
 			continue
 		}

+		if s.forwardToTSOService() {
+			if request.GetCount() == 0 {


Can we reuse forwardTSO here?

Let me think about how to reduce the duplicated code.

okJiang · 2024-10-09T07:28:35Z

server/config/config.go

@@ -855,12 +856,16 @@ func (c *DRAutoSyncReplicationConfig) adjust(meta *configutil.ConfigMetaData) {
 // MicroServiceConfig is the configuration for micro service.
 type MicroServiceConfig struct {
 	EnableSchedulingFallback bool `toml:"enable-scheduling-fallback" json:"enable-scheduling-fallback,string"`
+	EnableTSOFallback        bool `toml:"enable-tso-fallback" json:"enable-tso-fallback,string"`


Is this neccesary? The code about this feature is so big that, I think, this switch can't control them as we expect.

Yes, if we don't want PD to provide the service after tso server is stopped.

okJiang · 2024-10-09T07:29:42Z

server/cluster/cluster.go

+	if c.IsServiceIndependent(constant.TSOServiceName) {
+		log.Info("PD server starts to provide timestamp")
+	}
+	c.UnsetServiceIndependent(constant.TSOServiceName)


Maybe we can add more comments for independentServices sync.Map

okJiang · 2024-10-09T07:55:23Z

tests/integrations/mcs/tso/server_test.go

+	ch := make(chan struct{})
+	ch1 := make(chan struct{})
+	wg.Add(1)
+	go func(ctx context.Context, wg *sync.WaitGroup, ch, ch1 chan struct{}) {


Seems we needn't delivery these params

okJiang · 2024-10-09T07:55:33Z

tests/integrations/mcs/tso/server_test.go

@@ -578,3 +585,212 @@ func (suite *CommonTestSuite) TestBootstrapDefaultKeyspaceGroup() {
 	suite.pdLeader.ResignLeader()
 	suite.pdLeader = suite.cluster.GetServer(suite.cluster.WaitLeader())
 }
+
+func TestTSOServiceSwitch1(t *testing.T) {


Please add a description for these three UTs. Or make the UT name more meaningful?

okJiang · 2024-10-09T07:56:01Z

tests/integrations/mcs/tso/server_test.go

+	ch1 <- struct{}{}
+	<-ch


Prefer to wrap a function

Suggested change

ch1 <- struct{}{}

<-ch

waitOneTS := func() {

ch1 <- struct{}{}

<-ch

}

...

waitOneTS()

okJiang · 2024-10-09T07:56:15Z

tests/integrations/mcs/tso/server_test.go

+	re.NoError(err)
+	tsoCluster.WaitForDefaultPrimaryServing(re)
+
+	// Wait for TSO server to start and PD to detect it


Do we have some ways to detect this instead of Sleep

I don't have a better idea.

okJiang · 2024-10-09T07:56:26Z

tests/integrations/mcs/tso/server_test.go

+	tsoCluster.Destroy()
+
+	// Wait for the configuration change to take effect
+	time.Sleep(300 * time.Millisecond)


okJiang · 2024-10-09T07:56:32Z

tests/integrations/mcs/tso/server_test.go

+	re.NoError(failpoint.Disable("github.com/tikv/pd/client/fastUpdateServiceMode"))
+}
+
+func TestTSOServiceSwitch3(t *testing.T) {


What is the difference with the above two?

Signed-off-by: Ryan Leung <[email protected]>

ti-chi-bot · 2024-10-15T09:55:07Z

@rleungx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-realcluster-test	`064ca7b`	link	false	`/test pull-integration-realcluster-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

ti-chi-bot · 2024-10-18T10:03:22Z

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-sigs/prow repository.

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 12, 2024

rleungx requested review from JmPotato, nolouch and HuSharp August 14, 2024 08:19

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2024

JmPotato reviewed Aug 15, 2024

View reviewed changes

rleungx force-pushed the dynamic-switch branch from 6c98636 to dc6b4a7 Compare August 21, 2024 06:36

rleungx force-pushed the dynamic-switch branch 2 times, most recently from a36ad5d to f2c0c14 Compare August 23, 2024 07:38

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 26, 2024

rleungx force-pushed the dynamic-switch branch from 5d11416 to 12c7821 Compare August 27, 2024 06:30

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

okJiang reviewed Aug 29, 2024

View reviewed changes

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 2, 2024

rleungx force-pushed the dynamic-switch branch from 96c89a1 to 0a659c8 Compare September 9, 2024 08:05

rleungx requested review from JmPotato and okJiang September 9, 2024 08:05

rleungx force-pushed the dynamic-switch branch from 0a659c8 to 80133c9 Compare September 10, 2024 07:17

rleungx force-pushed the dynamic-switch branch from 80133c9 to e3f29e6 Compare September 29, 2024 09:43

ti-chi-bot bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 29, 2024

rleungx added 12 commits October 8, 2024 15:36

discover faster

0e97123

Signed-off-by: Ryan Leung <[email protected]>

forward request without recreating stream

3c022dd

Signed-off-by: Ryan Leung <[email protected]>

fix

2019613

Signed-off-by: Ryan Leung <[email protected]>

fix

91858ed

Signed-off-by: Ryan Leung <[email protected]>

fix

c081428

Signed-off-by: Ryan Leung <[email protected]>

fix failpoint

47bbe44

Signed-off-by: Ryan Leung <[email protected]>

fix

773774f

Signed-off-by: Ryan Leung <[email protected]>

address comments

2f2b738

Signed-off-by: Ryan Leung <[email protected]>

address the comment

f33e797

Signed-off-by: Ryan Leung <[email protected]>

resolve conflicts

66f5936

Signed-off-by: Ryan Leung <[email protected]>

remove unstable test

c6d90e5

Signed-off-by: Ryan Leung <[email protected]>

add config test

34e7a75

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the dynamic-switch branch from df3a3ec to 403519c Compare October 8, 2024 07:36

disable tso fallback by default

039088d

Signed-off-by: Ryan Leung <[email protected]>

rleungx force-pushed the dynamic-switch branch from 403519c to 039088d Compare October 8, 2024 07:45

refactor and test

72fc9ed

Signed-off-by: Ryan Leung <[email protected]>

okJiang reviewed Oct 8, 2024

View reviewed changes

rleungx added 2 commits October 8, 2024 18:19

fix

e27ab1f

Signed-off-by: Ryan Leung <[email protected]>

fix

d5389fc

Signed-off-by: Ryan Leung <[email protected]>

okJiang reviewed Oct 9, 2024

View reviewed changes

rleungx added 4 commits October 9, 2024 16:47

address comments

18a8c02

Signed-off-by: Ryan Leung <[email protected]>

reduce the duplicated code

d7a88d8

Signed-off-by: Ryan Leung <[email protected]>

fix

edf3908

Signed-off-by: Ryan Leung <[email protected]>

adjust check tso service

064ca7b

Signed-off-by: Ryan Leung <[email protected]>

rleungx mentioned this pull request Oct 16, 2024

*: move tso to independent thread #8720

Open

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

	func (c *client) setServiceMode(newMode pdpb.ServiceMode, skipSameMode bool) {
	func (c *client) setServiceMode(newMode pdpb.ServiceMode, force bool) {

	// IsServiceModeChange will determine whether there is a service mode change.
	// IsServiceModeChange determines whether there is a service mode change.

-	if s.forwardToTSOService() {
+	if !s.forwardToTSOService() {
+        return s.tsoAllocatorManager.HandleRequest(ctx, tso.GlobalDCLocation, 1)
+    }
+    request := xxxx
+    .....

*: support dynamic tso service #8517

Are you sure you want to change the base?

*: support dynamic tso service #8517

Conversation

rleungx commented Aug 12, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

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

codecov bot commented Aug 21, 2024 • 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

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

ti-chi-bot bot commented Sep 6, 2024

okJiang Oct 8, 2024 • 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

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

ti-chi-bot bot commented Oct 15, 2024

ti-chi-bot bot commented Oct 18, 2024

rleungx commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 21, 2024 •

edited

Loading

okJiang Oct 8, 2024 •

edited

Loading