kv request: configurable KV Timeout by hihihuhu · Pull Request #45601 · pingcap/tidb

hihihuhu · 2023-07-26T20:06:04Z

What problem does this PR solve?

Issue Number: close #45380

Problem Summary:

What is changed and how it works?

include:

Add new session variable tidb_kv_read_timeout.
Support hint processing for tidb_kv_read_timeout. When parsing and processing user hints in the ExtractTableHintsFromStmtNode and handleStmtHints functions, convert the input hint value into the corresponding field of StmtContext so it can be used later.
Add the KVReadTimeout field in the StatementContext, it could be set from the session variable or the hint value.
Support timeout processing logic in the copTask, setting the timeout value to the Context.max_execution_duration_ms field when building coprocessor kv-reuqests.

not yet finished:

Support timeout processing logic in the pointGetExecutor and batchPointGetExecutor, setting the timeout value to the underlying KVSnapshot which is used by the executors.
waiting for client-go implementation, adding a TODO for now

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

cc @Tema

ti-chi-bot · 2023-07-26T20:06:14Z

Hi @hihihuhu. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

tiprow · 2023-07-26T20:06:28Z

Hi @hihihuhu. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

hawkingrei · 2023-07-27T01:20:38Z

/ok-to-test

hawkingrei · 2023-07-27T01:22:32Z

/test all

ti-chi-bot · 2023-07-27T01:22:34Z

@hawkingrei: No presubmit jobs available for pingcap/tidb@dev_tidb_kv_read_timeout

Details

In response to this:

/test all

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.

tiprow · 2023-07-27T01:22:45Z

@hawkingrei: No presubmit jobs available for pingcap/tidb@dev_tidb_kv_read_timeout

Details

In response to this:

/test all

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.

hawkingrei · 2023-07-27T01:26:52Z

@hihihuhu Can you change the target base branch to master?

cfzjywxk · 2023-07-27T01:55:40Z

@hihihuhu Can you change the target base branch to master?

@hawkingrei Currently, we are actively developing in the development branch, and once the features are fully refined, they will be merged into the master branch.

hawkingrei · 2023-07-27T02:32:58Z

/test all

tiprow · 2023-07-27T02:33:16Z

@hawkingrei: No presubmit jobs available for pingcap/tidb@feature/dev_tidb_kv_read_timeout

Details

In response to this:

/test all

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.

codecov · 2023-07-27T02:47:59Z

Codecov Report

Merging #45601 (fd1a2d9) into master (9d517f6) will decrease coverage by 1.5570%.
Report is 2 commits behind head on master.
The diff coverage is 75.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #45601        +/-   ##
================================================
- Coverage   74.7531%   73.1961%   -1.5570%     
================================================
  Files          1293       1300         +7     
  Lines        398022     400215      +2193     
================================================
- Hits         297534     292942      -4592     
- Misses        81121      88710      +7589     
+ Partials      19367      18563       -804

Flag	Coverage Δ
integration	`26.2506% <44.0000%> (-20.0126%)`	⬇️
unit	`73.4612% <95.0000%> (+0.1233%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0656% <100.0000%> (+0.0162%)`	⬆️
br	`50.8191% <ø> (-13.6745%)`	⬇️

Signed-off-by: crazycs520 <crazycs520@gmail.com>

…b_kv_read_timeout

ti-chi-bot · 2023-08-11T06:44:38Z

[LGTM Timeline notifier]

Timeline:

2023-08-09 13:22:46.355318488 +0000 UTC m=+118930.904334460: ☑️ agreed by you06.
2023-08-11 06:44:37.031949547 +0000 UTC m=+267841.580965534: ☑️ agreed by crazycs520.

crazycs520 · 2023-08-11T06:53:16Z

/retest

easonn7 · 2023-08-11T06:56:25Z

/approve

discussed with @cfzjywxk , LGTM

ti-chi-bot · 2023-08-11T06:56:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crazycs520, easonn7, you06

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [crazycs520,you06]
~~parser/OWNERS~~ [easonn7]
~~sessionctx/variable/OWNERS~~ [easonn7]

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

Signed-off-by: crazycs520 <crazycs520@gmail.com>

crazycs520 · 2023-08-11T07:34:53Z

/retest

crazycs520 · 2023-08-11T07:56:51Z

/retest

…b_kv_read_timeout

hawkingrei · 2023-08-11T10:44:58Z

/retest

hawkingrei · 2023-08-11T11:22:01Z

/retest

hawkingrei · 2023-08-11T12:01:53Z

/retest

close pingcap#45380

* kv request: configurable KV Timeout (#45601) close #45380 * update go.mod Signed-off-by: crazycs520 <crazycs520@gmail.com> * refine code Signed-off-by: crazycs520 <crazycs520@gmail.com> * make bazel_prepare Signed-off-by: crazycs520 <crazycs520@gmail.com> * update go.mod Signed-off-by: crazycs520 <crazycs520@gmail.com> * update go.mod Signed-off-by: crazycs520 <crazycs520@gmail.com> --------- Signed-off-by: crazycs520 <crazycs520@gmail.com> Co-authored-by: Chen Ding <dingc05@gmail.com>

hihihuhu requested a review from a team as a code owner July 26, 2023 20:06

ti-chi-bot bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2023

ti-chi-bot bot added the needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. label Jul 26, 2023

hihihuhu changed the title ~~Configurable KV Timeout~~ kv request: configurable KV Timeout Jul 26, 2023

ti-chi-bot bot removed the do-not-merge/invalid-title label Jul 26, 2023

hihihuhu force-pushed the dev_tidb_kv_read_timeout branch from d644eb2 to ca0f947 Compare July 26, 2023 21:47

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jul 27, 2023

cfzjywxk requested review from cfzjywxk, crazycs520, ekexium, you06 and zyguan July 27, 2023 01:56

cfzjywxk added type/enhancement The issue or PR belongs to an enhancement. type/new-feature feature/developing the related feature is in development labels Jul 27, 2023

hihihuhu force-pushed the dev_tidb_kv_read_timeout branch from ca0f947 to fde6a95 Compare July 27, 2023 17:45

update go.mod

1f03df3

Signed-off-by: crazycs520 <crazycs520@gmail.com>

crazycs520 removed the do-not-merge/needs-linked-issue label Aug 11, 2023

crazycs520 added 2 commits August 11, 2023 14:41

add more test

41479a3

Signed-off-by: crazycs520 <crazycs520@gmail.com>

Merge branch 'master' of https://github.com/pingcap/tidb into dev_tid…

fb5a7ab

…b_kv_read_timeout

crazycs520 force-pushed the dev_tidb_kv_read_timeout branch from cc4381d to fb5a7ab Compare August 11, 2023 06:43

ti-chi-bot bot added do-not-merge/needs-linked-issue and removed do-not-merge/needs-linked-issue labels Aug 11, 2023

crazycs520 approved these changes Aug 11, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 11, 2023

ti-chi-bot bot added the approved label Aug 11, 2023

make bazel_prepare

2c8d1e2

Signed-off-by: crazycs520 <crazycs520@gmail.com>

Merge branch 'master' of https://github.com/pingcap/tidb into dev_tid…

fd1a2d9

…b_kv_read_timeout

ti-chi-bot bot merged commit 0a5e0b3 into pingcap:master Aug 11, 2023

crazycs520 pushed a commit to crazycs520/tidb that referenced this pull request Aug 21, 2023

kv request: configurable KV Timeout (pingcap#45601)

e9c1e53

close pingcap#45380

crazycs520 mentioned this pull request Aug 21, 2023

kv request: configurable KV Timeout (#45601) #46274

Merged

12 tasks

This was referenced Sep 4, 2023

*: add doc for tikv_client_read_timeout system variable pingcap/docs-cn#14964

Merged

rename variable tidb_kv_read_timeout to tikv_client_read_timeout and remove tidb_kv_read_timeout stmt hint #46713

Closed

Conversation

hihihuhu commented Jul 26, 2023 • edited by crazycs520 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Jul 26, 2023

Uh oh!

tiprow bot commented Jul 26, 2023

Uh oh!

hawkingrei commented Jul 27, 2023

Uh oh!

hawkingrei commented Jul 27, 2023

Uh oh!

ti-chi-bot bot commented Jul 27, 2023

Uh oh!

tiprow bot commented Jul 27, 2023

Uh oh!

hawkingrei commented Jul 27, 2023

Uh oh!

cfzjywxk commented Jul 27, 2023

Uh oh!

hawkingrei commented Jul 27, 2023

Uh oh!

tiprow bot commented Jul 27, 2023

Uh oh!

codecov bot commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ti-chi-bot bot commented Aug 11, 2023

[LGTM Timeline notifier]

Uh oh!

crazycs520 commented Aug 11, 2023

Uh oh!

easonn7 commented Aug 11, 2023

Uh oh!

ti-chi-bot bot commented Aug 11, 2023

Uh oh!

crazycs520 commented Aug 11, 2023

Uh oh!

crazycs520 commented Aug 11, 2023

Uh oh!

hawkingrei commented Aug 11, 2023

Uh oh!

hawkingrei commented Aug 11, 2023

Uh oh!

hawkingrei commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hihihuhu commented Jul 26, 2023 •

edited by crazycs520

Loading

codecov bot commented Jul 27, 2023 •

edited

Loading