client: Support parallel TSO RPC requests on single dispatcher loop #8633

MyonKeminta · 2024-09-14T11:02:04Z

What problem does this PR solve?

Issue Number: Close #8432

What is changed and how does it work?

client: Support parallel TSO RPC requests on single dispatcher loop

This commit supports handling multiple TSO RPC concurrently in one single dispatcher loop to reduce the expected time that each GetTS call spent on waiting the next batch.

Check List

Tests

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

Code changes

Has the configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
PR to update pingcap/tiup:
Need to cherry-pick to the release branch

Release note

None.

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…make-tso-stream-async

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…make-tso-stream-async

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…make-tso-stream-async

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…make-tso-stream-async

…Request from the pool Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

…stream

…l-tso-requests-in-one-stream

ti-chi-bot · 2024-09-14T11:02:07Z

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

MyonKeminta · 2024-09-23T12:03:10Z

/retest

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

JmPotato · 2024-09-25T03:14:04Z

client/tso_dispatcher.go

+		immediatelyUpdate = true
+	})
+
+	if !immediatelyUpdate && now.Sub(td.lastCheckConcurrencyTime) < dispatcherCheckRPCConcurrencyInterval {


Don't we need to update td.lastCheckConcurrencyTime here also?

It's updated at line 701. We don't update it if it's skipped, otherwise it may never be checked

JmPotato · 2024-09-25T03:16:30Z

client/tso_stream.go

@@ -228,6 +232,8 @@ var streamIDAlloc atomic.Int32

 const invalidStreamID = "<invalid>"

+const maxPendingRequestsInTSOStream = 64


Please merge with line 233.

JmPotato · 2024-09-25T03:17:45Z

client/tso_dispatcher.go

 		// last time.
-		if tsoutil.TSLessEqual(physical, firstLogical, lastTSOInfo.physical, lastTSOInfo.logical) {
+		if tsoutil.TSLessEqual(curTSOInfo.physical, firstLogical, lastTSOInfo.physical, lastTSOInfo.logical) {
 			log.Panic("[tso] timestamp fallback",


Shall we print out the stream ID here?

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

codecov · 2024-09-25T05:05:34Z

Codecov Report

Attention: Patch coverage is 58.24742% with 81 lines in your changes missing coverage. Please review.

Project coverage is 77.58%. Comparing base (f3e9d9a) to head (8f45037).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8633      +/-   ##
==========================================
- Coverage   77.64%   77.58%   -0.07%     
==========================================
  Files         474      474              
  Lines       62112    62311     +199     
==========================================
+ Hits        48228    48345     +117     
- Misses      10333    10399      +66     
- Partials     3551     3567      +16

Flag	Coverage Δ
unittests	`77.58% <58.24%> (-0.07%)`	⬇️

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

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta · 2024-09-25T10:34:22Z

/retest

ti-chi-bot · 2024-09-25T10:47:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JmPotato, lhy1024

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JmPotato,lhy1024]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-09-25T10:47:51Z

[LGTM Timeline notifier]

Timeline:

2024-09-25 09:08:57.119133261 +0000 UTC m=+1643406.859557201: ☑️ agreed by JmPotato.
2024-09-25 10:47:50.586981506 +0000 UTC m=+1649340.327405444: ☑️ agreed by lhy1024.

MyonKeminta · 2024-09-25T11:24:03Z

/retest

MyonKeminta added 29 commits August 5, 2024 17:32

Add implementation of making tsoStream asynchronous

140a7c2

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add basic tests for rewritten tsoStream

88d9f5b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add a concurrency test

623d896

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix some of CI failures

86d0f42

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix integration tests

ff0f11a

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix tso deadline

ef47b9d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix stream broken handling

ff1d50d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix comments

9076c9d

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

try to avoid double invocation to the callback

39e4e0b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix lint

632781b

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Address comments

8809c45

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Pass context outside the tsoStream (push for running CI)

ca64ed0

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

client: Add benchmark for tsoStream and dispatcher

67967e4

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

1362aac

…make-tso-stream-async

Fix build

4699b41

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Reimplement mock receiving

7dbcd77

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

f4dc99a

…make-tso-stream-async

Fix disptacher benchmark

1f2dcd8

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix incorrect error generating

123a869

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

2c6aaba

…make-tso-stream-async

Disable logs in BenchmarkTSOStreamSendRecv

bff9eae

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix lint

ef6e365

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge branch 'm/add-benchmarks-for-tso-stream-and-dispatcher' into m/…

a9f4aee

…make-tso-stream-async

Make the comments more clear; unset req.streamID when getting new tso…

388d705

…Request from the pool Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix lint

a183aca

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Add latency estimation

4296c3f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Support concurrent RPC

9b5b054

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Merge commit '098b802fc' into m/support-parallel-tso-requests-in-one-…

c44339f

…stream

Merge remote-tracking branch 'upstream/master' into m/support-paralle…

364101f

…l-tso-requests-in-one-stream

Add tests for concurrency limiting

f451595

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 24, 2024

MyonKeminta added 2 commits September 25, 2024 04:10

Add tests for calculating batch delay time

708bbe9

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix lint

4a795fe

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta marked this pull request as ready for review September 24, 2024 20:14

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

JmPotato reviewed Sep 25, 2024

View reviewed changes

MyonKeminta added 2 commits September 25, 2024 12:34

Address comments; add more comments to explain checkMonotonicity

0bfe55f

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

Fix data race

0bfe9a6

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/support-parallel-tso-requests-in-one-stream branch from e26315b to 0bfe9a6 Compare September 25, 2024 04:58

MyonKeminta mentioned this pull request Sep 25, 2024

*: Add sysvar tidb_tso_client_rpc_mode for setting TSO RPC concurrency pingcap/tidb#56292

Merged

13 tasks

JmPotato approved these changes Sep 25, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 25, 2024

Update estimated latency metric in tsoStream instead of tsoDispatcher

f67467e

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>

MyonKeminta force-pushed the m/support-parallel-tso-requests-in-one-stream branch from af1c3d2 to f67467e Compare September 25, 2024 09:31

lhy1024 approved these changes Sep 25, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 25, 2024

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 25, 2024

Merge branch 'master' into m/support-parallel-tso-requests-in-one-stream

8f45037

ti-chi-bot bot merged commit 642f0e9 into tikv:master Sep 26, 2024
23 of 25 checks passed

MyonKeminta deleted the m/support-parallel-tso-requests-in-one-stream branch September 26, 2024 02:54

MyonKeminta mentioned this pull request Nov 27, 2024

High P99 Latency with tsoRequestDispatcher tikv/client-go#1257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Support parallel TSO RPC requests on single dispatcher loop #8633

client: Support parallel TSO RPC requests on single dispatcher loop #8633

MyonKeminta commented Sep 14, 2024

ti-chi-bot bot commented Sep 14, 2024

MyonKeminta commented Sep 23, 2024

JmPotato Sep 25, 2024

MyonKeminta Sep 25, 2024

JmPotato Sep 25, 2024

JmPotato Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

MyonKeminta commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

MyonKeminta commented Sep 25, 2024

		@@ -228,6 +232,8 @@ var streamIDAlloc atomic.Int32

		const invalidStreamID = "<invalid>"

		const maxPendingRequestsInTSOStream = 64

client: Support parallel TSO RPC requests on single dispatcher loop #8633

client: Support parallel TSO RPC requests on single dispatcher loop #8633

Conversation

MyonKeminta commented Sep 14, 2024

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Sep 14, 2024

MyonKeminta commented Sep 23, 2024

JmPotato Sep 25, 2024

Choose a reason for hiding this comment

MyonKeminta Sep 25, 2024

Choose a reason for hiding this comment

JmPotato Sep 25, 2024

Choose a reason for hiding this comment

JmPotato Sep 25, 2024

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

MyonKeminta commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

ti-chi-bot bot commented Sep 25, 2024

[LGTM Timeline notifier]

MyonKeminta commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading