use tidb_kv_read_timeout as first kv request timeout #919

crazycs520 · 2023-07-28T07:01:01Z

issue: pingcap/tidb#45380
related TiDB PR: pingcap/tidb#45601

you06 · 2023-08-02T09:20:03Z

internal/locate/region_request.go

+		return nil, err
+	}
+	replicaRead := selector.targetIdx != state.leaderIdx
+	rpcCtx.contextPatcher.replicaRead = &replicaRead


Also need to set rpcCtx.contextPatcher.staleRead to false, unless stale read will be sent to follower, may avoid this because the tryFollower is already a backoff.

And #916 made some changes of tryFollower, what about picking and working above it?

internal/client/client_batch.go

you06 · 2023-08-02T12:09:17Z

internal/locate/region_request.go

+			// TODO(crazycs520): need discuss.
+			if accessLeader, ok := s.replicaSelector.state.(*accessKnownLeader); ok {
+				s.replicaSelector.state = &tryFollower{leaderIdx: accessLeader.leaderIdx, lastIdx: accessLeader.leaderIdx}
+			}


I think it's ok to change state for accessKnownLeader here, accessFollower can make state change in it's next when leader is exhausted.

internal/locate/region_request.go

txnkv/txnsnapshot/snapshot.go

internal/locate/region_request3_test.go

internal/locate/region_request.go

cfzjywxk · 2023-08-10T07:16:38Z

internal/locate/region_request.go

 }

 func (state *tryFollower) next(bo *retry.Backoffer, selector *replicaSelector) (*RPCContext, error) {
 	var targetReplica *replica
+	hasDeadlineExceededErr := false


Ditto, we may need a way to distinguish timeout error using input timeout value or the default timeout value, maybe abstract another error type for the configurable timeout error?

internal/locate/region_request.go

cfzjywxk · 2023-08-10T07:46:45Z

internal/locate/region_request.go

@@ -1767,10 +1812,16 @@ func regionErrorToLabel(e *errorpb.Error) string {
 		return "flashback_not_prepared"
 	} else if e.GetIsWitness() != nil {
 		return "peer_is_witness"
+	} else if isDeadlineExceeded(e) {


Maybe we need to distinguish the default KvTimeout and ConfigurableKvTimeout errors.
@you06 What do you think?

Signed-off-by: crazycs520 <[email protected]>

internal/locate/region_request3_test.go

Signed-off-by: crazycs520 <[email protected]>

* client-go: add some key range info to error when PD returned no region (#862) Signed-off-by: Chao Wang <[email protected]> * *: refine non-global stale-read request retry logic (#863) Signed-off-by: crazycs520 <[email protected]> * Fix the issue that primary pessimistic lock may be left not cleared after GC (#866) * Fix the issue that primary pessimistic lock may be left not cleared after GC Signed-off-by: MyonKeminta <[email protected]> * Fix mysteriously shown up thing that makes compilation failed Signed-off-by: MyonKeminta <[email protected]> * Fix test effectiveness (forgot to set txn2 to pessimistic txn); add more strict checks Signed-off-by: MyonKeminta <[email protected]> * Address comments Signed-off-by: MyonKeminta <[email protected]> --------- Signed-off-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]> * add explicit request source type to label the external request like lightning/br (#868) Signed-off-by: nolouch <[email protected]> * use '%d' instead of '%q' for some int values in error message (#875) Signed-off-by: Chao Wang <[email protected]> * format key in error message in method `scanRegions` (#876) Signed-off-by: Chao Wang <[email protected]> * make cop request timeout a config paramter (#865) * update Signed-off-by: Spade A <[email protected]> * update Signed-off-by: Spade A <[email protected]> * update Signed-off-by: Spade A <[email protected]> * update Signed-off-by: Spade A <[email protected]> --------- Signed-off-by: Spade A <[email protected]> * region_cache: support check pending tiflash peer (#821) Signed-off-by: guo-shaoge <[email protected]> Co-authored-by: disksing <[email protected]> * *: add `SnapshotIterReverse` and make `iterReverse` supports `lowerBound` (#883) Signed-off-by: Jason Mo <[email protected]> * *: fix stale read ops metric (#878) (#889) Signed-off-by: crazycs520 <[email protected]> Co-authored-by: disksing <[email protected]> * add gc options (#828) Signed-off-by: weedge <[email protected]> Co-authored-by: disksing <[email protected]> * reload region cache when store is resolved from invalid status (#843) Signed-off-by: you06 <[email protected]> Co-authored-by: disksing <[email protected]> * ci: update setup-go action (#904) Signed-off-by: disksing <[email protected]> * fix unexpected slow query during GC running after stop 1 tikv-server (#899) (#909) * fix unexpected slow query during GC running after stop 1 tikv-server Signed-off-by: crazycs520 <[email protected]> * fix test Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]> * resource_manager: ignore ru metrics for background request (#872) Signed-off-by: husharp <[email protected]> Co-authored-by: disksing <[email protected]> * add more log for diagnose (#915) * add more log for diagnose Signed-off-by: crazycs520 <[email protected]> * fix Signed-off-by: crazycs520 <[email protected]> * add more log for diagnose Signed-off-by: crazycs520 <[email protected]> * add more log Signed-off-by: crazycs520 <[email protected]> * address comment Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]> * use context logger as much as possible (#908) * use context logger as much as possible Signed-off-by: crazycs520 <[email protected]> * refine Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]> * Resume max retry time check for stale read retry with leader option(#903) (#911) * Resume max retry time check for stale read retry with leader option Signed-off-by: cfzjywxk <[email protected]> * add cancel Signed-off-by: cfzjywxk <[email protected]> --------- Signed-off-by: cfzjywxk <[email protected]> * request_source: remove default label (#890) * request_source: remove default label Signed-off-by: nolouch <[email protected]> * add a function to set request source task type (#925) * add a function to set request source task type Signed-off-by: glorv <[email protected]> * ci: update go version (#936) * ci: update go version Signed-off-by: crazycs520 <[email protected]> * fix test Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]> * use tidb_kv_read_timeout as first kv request timeout (#919) * support tidb_kv_read_timeout as first round kv request timeout Signed-off-by: crazycs520 <[email protected]> * fix ci Signed-off-by: crazycs520 <[email protected]> * fix ci Signed-off-by: crazycs520 <[email protected]> * fix ci Signed-off-by: crazycs520 <[email protected]> * fix ci Signed-off-by: crazycs520 <[email protected]> * fix ci Signed-off-by: crazycs520 <[email protected]> * update comment Signed-off-by: crazycs520 <[email protected]> * refine test Signed-off-by: crazycs520 <[email protected]> --------- Signed-off-by: crazycs520 <[email protected]> * [pick] resource_control: bypass some internal urgent request (#938) * resource_control: bypass some internal urgent request (#884) Signed-off-by: nolouch <[email protected]> * resourcecontrol: fix nil pointer (#900) Signed-off-by: nolouch <[email protected]> --------- Signed-off-by: nolouch <[email protected]> --------- Signed-off-by: Chao Wang <[email protected]> Signed-off-by: crazycs520 <[email protected]> Signed-off-by: MyonKeminta <[email protected]> Signed-off-by: nolouch <[email protected]> Signed-off-by: Spade A <[email protected]> Signed-off-by: guo-shaoge <[email protected]> Signed-off-by: Jason Mo <[email protected]> Signed-off-by: weedge <[email protected]> Signed-off-by: you06 <[email protected]> Signed-off-by: disksing <[email protected]> Signed-off-by: husharp <[email protected]> Signed-off-by: cfzjywxk <[email protected]> Signed-off-by: glorv <[email protected]> Signed-off-by: iosmanthus <[email protected]> Co-authored-by: 王超 <[email protected]> Co-authored-by: crazycs <[email protected]> Co-authored-by: MyonKeminta <[email protected]> Co-authored-by: MyonKeminta <[email protected]> Co-authored-by: ShuNing <[email protected]> Co-authored-by: Spade A <[email protected]> Co-authored-by: guo-shaoge <[email protected]> Co-authored-by: disksing <[email protected]> Co-authored-by: Hangjie Mo <[email protected]> Co-authored-by: weedge <[email protected]> Co-authored-by: you06 <[email protected]> Co-authored-by: Hu# <[email protected]> Co-authored-by: cfzjywxk <[email protected]> Co-authored-by: glorv <[email protected]>

--------- Signed-off-by: crazycs520 <[email protected]>

Signed-off-by: crazycs520 <[email protected]>

crazycs520 mentioned this pull request Jul 31, 2023

Dev tidb kv read timeout pingcap/tidb#45697

Closed

crazycs520 marked this pull request as ready for review August 2, 2023 07:26

crazycs520 changed the base branch from feature/dev_tidb_kv_read_timeout to master August 2, 2023 11:11

you06 reviewed Aug 2, 2023

View reviewed changes

crazycs520 mentioned this pull request Aug 7, 2023

Configurable KV Timeout Tracking Issue pingcap/tidb#45380

Closed

9 tasks

cfzjywxk requested review from cfzjywxk, zyguan, MyonKeminta and ekexium August 7, 2023 07:50

you06 reviewed Aug 8, 2023

View reviewed changes

internal/locate/region_request.go Outdated Show resolved Hide resolved

you06 reviewed Aug 8, 2023

View reviewed changes

internal/locate/region_request.go Outdated Show resolved Hide resolved

you06 approved these changes Aug 9, 2023

View reviewed changes

cfzjywxk reviewed Aug 10, 2023

View reviewed changes

cfzjywxk approved these changes Aug 10, 2023

View reviewed changes

support tidb_kv_read_timeout as first round kv request timeout

f54b182

Signed-off-by: crazycs520 <[email protected]>

crazycs520 force-pushed the use_tidb_kv_read_timeout branch from 6a8e100 to f54b182 Compare August 10, 2023 12:01

Merge branch 'master' into use_tidb_kv_read_timeout

497db34

crazycs520 force-pushed the use_tidb_kv_read_timeout branch from 38e349d to 497db34 Compare August 10, 2023 16:17

crazycs520 added 5 commits August 11, 2023 00:21

fix ci

6fcf70f

Signed-off-by: crazycs520 <[email protected]>

fix ci

bb71f08

Signed-off-by: crazycs520 <[email protected]>

fix ci

70cab5c

Signed-off-by: crazycs520 <[email protected]>

fix ci

47d8bf3

Signed-off-by: crazycs520 <[email protected]>

fix ci

d439bfa

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk reviewed Aug 11, 2023

View reviewed changes

internal/locate/region_request3_test.go Outdated Show resolved Hide resolved

internal/locate/region_request3_test.go Outdated Show resolved Hide resolved

crazycs520 added 2 commits August 11, 2023 10:06

update comment

1202080

Signed-off-by: crazycs520 <[email protected]>

refine test

8bd26b0

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk approved these changes Aug 11, 2023

View reviewed changes

cfzjywxk merged commit 8a21440 into tikv:master Aug 11, 2023

crazycs520 added a commit to crazycs520/client-go that referenced this pull request Aug 17, 2023

use tidb_kv_read_timeout as first kv request timeout (tikv#919)

4900b27

--------- Signed-off-by: crazycs520 <[email protected]>

crazycs520 mentioned this pull request Aug 17, 2023

use tidb_kv_read_timeout as first kv request timeout (#919) #948

Merged

crazycs520 added a commit to crazycs520/client-go that referenced this pull request Aug 22, 2023

use tidb_kv_read_timeout as first kv request timeout (tikv#919)

1046d55

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk pushed a commit that referenced this pull request Aug 23, 2023

use tidb_kv_read_timeout as first kv request timeout (#919) (#948)

25cbaa3

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk pushed a commit that referenced this pull request Aug 29, 2023

use tidb_kv_read_timeout as first kv request timeout (#919) (#948)

dfae543

Signed-off-by: crazycs520 <[email protected]>

cfzjywxk mentioned this pull request Aug 29, 2023

[DNM]: for test #955

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use tidb_kv_read_timeout as first kv request timeout #919

use tidb_kv_read_timeout as first kv request timeout #919

crazycs520 commented Jul 28, 2023 •

edited

Loading

you06 Aug 2, 2023

crazycs520 Aug 8, 2023

you06 Aug 2, 2023

cfzjywxk Aug 10, 2023

cfzjywxk Aug 10, 2023

use tidb_kv_read_timeout as first kv request timeout #919

use tidb_kv_read_timeout as first kv request timeout #919

Conversation

crazycs520 commented Jul 28, 2023 • edited Loading

you06 Aug 2, 2023

Choose a reason for hiding this comment

crazycs520 Aug 8, 2023

Choose a reason for hiding this comment

you06 Aug 2, 2023

Choose a reason for hiding this comment

cfzjywxk Aug 10, 2023

Choose a reason for hiding this comment

cfzjywxk Aug 10, 2023

Choose a reason for hiding this comment

crazycs520 commented Jul 28, 2023 •

edited

Loading