client: fix Stream timeout logic #5551

CabinfeverB · 2022-09-26T10:49:31Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: Close #5207

What is changed and how does it work?

fix Stream timeout logic

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

After the leader recovers the service, the interval of client tso service recovery is tested. Compared with the previous worst-case recovery time of 23 seconds, the current recovery time can be controlled within one second
before

This PR

Side effects

Possible performance regression

Related changes

Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2022-09-26T10:49:33Z

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

codecov · 2022-09-26T10:58:58Z

Codecov Report

Base: 75.71% // Head: 75.68% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (1e9e9b8) compared to base (e0061e3).
Patch coverage: 65.21% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5551      +/-   ##
==========================================
- Coverage   75.71%   75.68%   -0.03%     
==========================================
  Files         326      326              
  Lines       32239    32243       +4     
==========================================
- Hits        24409    24403       -6     
- Misses       5715     5725      +10     
  Partials     2115     2115

Flag	Coverage Δ
unittests	`75.68% <65.21%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
client/client.go	`68.91% <65.21%> (-0.72%)`	⬇️
server/schedulers/shuffle_hot_region.go	`55.55% <0.00%> (-10.11%)`	⬇️
server/storage/kv/etcd_kv.go	`78.26% <0.00%> (-7.25%)`	⬇️
server/tso/tso.go	`66.85% <0.00%> (-5.06%)`	⬇️
tools/pd-ctl/pdctl/command/operator.go	`66.66% <0.00%> (-1.15%)`	⬇️
pkg/dashboard/adapter/manager.go	`78.16% <0.00%> (-1.15%)`	⬇️
server/schedule/hbstream/heartbeat_streams.go	`72.72% <0.00%> (-1.02%)`	⬇️
server/schedule/operator_controller.go	`86.42% <0.00%> (-0.91%)`	⬇️
server/tso/allocator_manager.go	`62.66% <0.00%> (-0.65%)`	⬇️
... and 21 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: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

Need to test.

nolouch · 2022-09-27T04:20:52Z

client/client.go

 	for i := 0; i < maxRetryTimes; i++ {
+		c.updateMember()


Consider use ScheduleCheckLeader when createTsoStream met error.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch · 2022-09-29T09:24:09Z

tests/client/client_test.go

@@ -347,6 +347,86 @@ func TestTSOFollowerProxy(t *testing.T) {
 	wg.Wait()
 }

+// TestUnavailableTimeAfterLeaderIsReady is used to test https://github.com/tikv/pd/issues/5207
+func TestUnavailableTimeAfterLeaderIsReady(t *testing.T) {
+	re := require.New(t)


How long does this test take to run?

I will paste a test result

rleungx · 2022-09-29T10:28:36Z

client/client.go

 				}
 			}
-			retryTimeConsuming = 0


Will the new implementation timeout immediately when back to streamChoosingLoop?

No, it will check whether stream is created. If created, it will not timeout

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-10-08T09:03:14Z

tests/client/client_test.go

+		cluster.RunServers([]*tests.TestServer{leader})
+	}()
+	wg.Wait()
+	t.Log(maxUnavailableTime, leaderReadyTime)


Do we need to log it?

rleungx · 2022-10-08T09:04:19Z

tests/client/client_test.go

+	go func() {
+		defer wg.Done()
+		var lastTS uint64
+		for i := 0; i < tsoRequestRound; i++ {
+			var physical, logical int64
+			var ts uint64
+			physical, logical, err = cli.GetTS(context.Background())
+			ts = tsoutil.ComposeTS(physical, logical)
+			if err != nil {
+				maxUnavailableTime = time.Now()
+				continue
+			}
+			re.NoError(err)
+			re.Less(lastTS, ts)
+			lastTS = ts
+		}
+	}()


How about defining a function?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

ti-chi-bot · 2022-10-10T14:10:47Z

In response to a cherrypick label: new pull request created: #5580.

ti-chi-bot · 2022-10-10T14:10:48Z

In response to a cherrypick label: new pull request created: #5581.

ti-chi-bot · 2022-10-10T14:10:48Z

In response to a cherrypick label: new pull request created: #5582.

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-10-10T14:10:49Z

In response to a cherrypick label: new pull request created: #5583.

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-10-10T14:10:50Z

In response to a cherrypick label: new pull request could not be created: failed to create pull request against tikv/pd#release-6.1 from head ti-chi-bot:cherry-pick-5551-to-release-6.1: the GitHub API request returns a 403 error: {"message":"You have exceeded a secondary rate limit and have been temporarily blocked from content creation. Please retry your request again later.","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#secondary-rate-limits"}

ti-chi-bot · 2022-10-10T14:10:50Z

In response to a cherrypick label: new pull request created: #5584.

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-10-13T07:35:01Z

In response to a cherrypick label: new pull request created: #5599.

close #5207, ref #5551 fix Stream timeout logic Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Yongbo Jiang <cabinfeveroier@gmail.com> Co-authored-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix

a2ed03b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added do-not-merge/needs-linked-issue release-note The PR should write the release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 26, 2022

ti-chi-bot requested review from JmPotato and rleungx September 26, 2022 10:49

CabinfeverB changed the title ~~fix~~ client: use Timer to check Stream timeout Sep 26, 2022

fix

cfc21ee

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB requested a review from nolouch September 27, 2022 02:20

nolouch reviewed Sep 27, 2022

View reviewed changes

CabinfeverB added 2 commits September 28, 2022 20:18

add test

5357ad9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add test

7ab2a0e

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Sep 29, 2022

View reviewed changes

rleungx reviewed Sep 29, 2022

View reviewed changes

fix

d58f9f3

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB changed the title ~~client: use Timer to check Stream timeout~~ client: fix Stream timeout logic Oct 8, 2022

ti-chi-bot added release-note-none and removed do-not-merge/needs-linked-issue release-note The PR should write the release note. labels Oct 8, 2022

CabinfeverB marked this pull request as ready for review October 8, 2022 08:43

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 Oct 8, 2022

rleungx reviewed Oct 8, 2022

View reviewed changes

address comment

321c3aa

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Oct 9, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 9, 2022

rleungx approved these changes Oct 9, 2022

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 Oct 9, 2022

CabinfeverB added the needs-cherry-pick-release-6.1 label Oct 10, 2022

ti-chi-bot mentioned this pull request Oct 10, 2022

client: fix Stream timeout logic (#5551) #5580

Closed

ti-chi-bot mentioned this pull request Oct 10, 2022

client: fix Stream timeout logic (#5551) #5581

Closed

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

9366683

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

64739a3

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

5eef731

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

This was referenced Oct 10, 2022

client: fix Stream timeout logic (#5551) #5582

Closed

client: fix Stream timeout logic (#5551) #5583

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

e8ebc3a

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Oct 10, 2022

client: fix Stream timeout logic (#5551) #5584

Merged

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

ea52272

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 10, 2022

This is an automated cherry-pick of tikv#5551

9baa5bb

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

CabinfeverB added needs-cherry-pick-release-6.1 and removed needs-cherry-pick-release-6.1 labels Oct 13, 2022

ti-chi-bot pushed a commit to ti-chi-bot/pd that referenced this pull request Oct 13, 2022

This is an automated cherry-pick of tikv#5551

469c82a

close tikv#5207 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Oct 13, 2022

client: fix Stream timeout logic (#5551) #5599

Merged

CabinfeverB removed needs-cherry-pick-release-5.0 The PR needs to cherry pick to release-5.0 branch. needs-cherry-pick-release-5.1 Type: Need cherry pick to release-5.1 needs-cherry-pick-release-5.2 Type: Need cherry pick to release-5.2 labels Nov 1, 2022

JmPotato deleted the client_tso_retry branch March 23, 2023 05:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: fix Stream timeout logic #5551

client: fix Stream timeout logic #5551

CabinfeverB commented Sep 26, 2022 •

edited

ti-chi-bot commented Sep 26, 2022 •

edited

codecov bot commented Sep 26, 2022 •

edited

nolouch left a comment •

edited

nolouch Sep 27, 2022 •

edited

nolouch Sep 29, 2022

CabinfeverB Sep 29, 2022

rleungx Sep 29, 2022 •

edited

CabinfeverB Oct 8, 2022

rleungx Oct 8, 2022

rleungx Oct 8, 2022

nolouch left a comment

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 13, 2022

client: fix Stream timeout logic #5551

client: fix Stream timeout logic #5551

Conversation

CabinfeverB commented Sep 26, 2022 • edited

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Sep 26, 2022 • edited

codecov bot commented Sep 26, 2022 • edited

Codecov Report

nolouch left a comment • edited

Choose a reason for hiding this comment

nolouch Sep 27, 2022 • edited

Choose a reason for hiding this comment

nolouch Sep 29, 2022

Choose a reason for hiding this comment

CabinfeverB Sep 29, 2022

Choose a reason for hiding this comment

rleungx Sep 29, 2022 • edited

Choose a reason for hiding this comment

CabinfeverB Oct 8, 2022

Choose a reason for hiding this comment

rleungx Oct 8, 2022

Choose a reason for hiding this comment

rleungx Oct 8, 2022

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 10, 2022

ti-chi-bot commented Oct 13, 2022

CabinfeverB commented Sep 26, 2022 •

edited

ti-chi-bot commented Sep 26, 2022 •

edited

codecov bot commented Sep 26, 2022 •

edited

nolouch left a comment •

edited

nolouch Sep 27, 2022 •

edited

rleungx Sep 29, 2022 •

edited