executor, store: Plumb the query max execution time to timebox the PD GetRegion grpc calls #56923

HaoW30 · 2024-10-28T16:15:49Z

What problem does this PR solve?

This change plumbs the TiDB query max execution time to the executor point_get(bath_point_get) and coprocessor(batch_coprocessor) tasks to timebox the GetRegion PD grpc calls (and other task operations).

Issue Number: ref #56753

Problem Summary:

What changed and how does it work?

Plumbed query execution time to kv request and distsql context
Set context deadline based on query max execution time for point get snapshot read (point_get and batch_point_get)
Set context deadline based on query max execution time for coprocessor tasks (coprocessor and batch_coprocessor)

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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

ti-chi-bot · 2024-10-28T16:15:59Z

Hi @HaoW30. 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.

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 · 2024-10-28T16:16:08Z

Hi @HaoW30. 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.

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 · 2024-11-05T02:39:09Z

@lhy1024: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

windtalker

lgtm

bb7133

LGTM

ti-chi-bot · 2024-11-07T18:40:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133, lhy1024, windtalker

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~~ [bb7133,windtalker]
~~pkg/distsql/OWNERS~~ [windtalker]

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-11-07T18:40:34Z

[LGTM Timeline notifier]

Timeline:

2024-11-07 06:15:24.95678577 +0000 UTC m=+1108037.795941315: ☑️ agreed by windtalker.
2024-11-07 18:40:33.292706861 +0000 UTC m=+1152746.131862407: ☑️ agreed by bb7133.

cfzjywxk · 2024-11-12T07:18:00Z

pkg/executor/batch_point_get.go

@@ -235,6 +236,10 @@ func (e *BatchPointGetExec) initialize(ctx context.Context) error {
 	var indexKeys []kv.Key
 	var err error
 	batchGetter := e.batchGetter
+	if e.Ctx().GetSessionVars().MaxExecutionTime > 0 {
+		// If MaxExecutionTime is set, we need to set the context deadline for the batch get.
+		ctx, _ = context.WithTimeout(ctx, time.Duration(e.Ctx().GetSessionVars().MaxExecutionTime)*time.Millisecond)


It is safer to to use

ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) defer cancel()

to follow the best practise and avoid unexpected resource usage risks.

yes, will do to avoid context leakage

cfzjywxk · 2024-11-12T07:21:08Z

pkg/distsql/request_builder.go

@@ -341,6 +341,7 @@ func (builder *RequestBuilder) SetFromSessionVars(dctx *distsqlctx.DistSQLContex
 	builder.Request.StoreBusyThreshold = dctx.LoadBasedReplicaReadThreshold
 	builder.Request.RunawayChecker = dctx.RunawayChecker
 	builder.Request.TiKVClientReadTimeout = dctx.TiKVClientReadTimeout
+	builder.Request.MaxExecutionTime = dctx.MaxExecutionTime


IMO adding a timeout to the context should be the final step. If we design with the expectation that the entire execution path checks for ctx.Done messages, perhaps we should review and document in the gitHub issue which tidb paths lack ctx.Done checks and list related TODO sub-tasks.

/cc @bb7133

cfzjywxk · 2024-11-12T07:21:19Z

pkg/executor/point_get.go

-	return e.snapshot.Get(ctx, key)
+	if e.Ctx().GetSessionVars().MaxExecutionTime > 0 {
+		// if the query has max execution time set, we need to set the context deadline for the get request
+		ctxWithTimeout, _ := context.WithTimeout(ctx, time.Duration(e.Ctx().GetSessionVars().MaxExecutionTime)*time.Millisecond)


wuhuizuo · 2024-11-15T03:18:59Z

/ok-to-test

…alls

* Add defer cancel to avoid memory leakage * Fix test context.TestContextDetach (pkg/distsql/context/context_test.go) * update bazel file

codecov · 2024-11-20T20:20:18Z

Codecov Report

Attention: Patch coverage is 30.76923% with 18 lines in your changes missing coverage. Please review.

Project coverage is 73.4422%. Comparing base (2ff351d) to head (8c021ab).
Report is 1 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56923        +/-   ##
================================================
+ Coverage   72.8166%   73.4422%   +0.6256%     
================================================
  Files          1676       1676                
  Lines        463603     463669        +66     
================================================
+ Hits         337580     340529      +2949     
+ Misses       105179     102371      -2808     
+ Partials      20844      20769        -75

Flag	Coverage Δ
integration	`43.4341% <7.6923%> (?)`
unit	`72.2582% <30.7692%> (+0.0505%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.3807% <ø> (-0.0181%)`	⬇️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

lhy1024 approved these changes Nov 5, 2024

View reviewed changes

windtalker approved these changes Nov 7, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. and removed do-not-merge/needs-tests-checked labels Nov 7, 2024

bb7133 approved these changes Nov 7, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 7, 2024

cfzjywxk reviewed Nov 12, 2024

View reviewed changes

ti-chi-bot bot requested a review from bb7133 November 12, 2024 07:21

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 Nov 15, 2024

Plumb the query max execution time to timebox the PD GetRegion grpc c…

beaa704

…alls

HaoW30 force-pushed the pdrpc-get-region-max-execution-time branch 2 times, most recently from e117b01 to ea29ba9 Compare November 20, 2024 19:29

Fixes for ci test failures

8c021ab

* Add defer cancel to avoid memory leakage * Fix test context.TestContextDetach (pkg/distsql/context/context_test.go) * update bazel file

HaoW30 force-pushed the pdrpc-get-region-max-execution-time branch from b5d8c33 to 8c021ab Compare November 20, 2024 20:00

ti-chi-bot bot merged commit c068b39 into pingcap:master Nov 20, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor, store: Plumb the query max execution time to timebox the PD GetRegion grpc calls #56923

executor, store: Plumb the query max execution time to timebox the PD GetRegion grpc calls #56923

HaoW30 commented Oct 28, 2024 •

edited by windtalker

Loading

ti-chi-bot bot commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

ti-chi-bot bot commented Nov 5, 2024

windtalker left a comment

bb7133 left a comment

ti-chi-bot bot commented Nov 7, 2024

ti-chi-bot bot commented Nov 7, 2024

cfzjywxk Nov 12, 2024

HaoW30 Nov 20, 2024

cfzjywxk Nov 12, 2024

cfzjywxk Nov 12, 2024

wuhuizuo commented Nov 15, 2024

codecov bot commented Nov 20, 2024 •

edited

Loading

executor, store: Plumb the query max execution time to timebox the PD GetRegion grpc calls #56923

executor, store: Plumb the query max execution time to timebox the PD GetRegion grpc calls #56923

Conversation

HaoW30 commented Oct 28, 2024 • edited by windtalker Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Oct 28, 2024

tiprow bot commented Oct 28, 2024

ti-chi-bot bot commented Nov 5, 2024

windtalker left a comment

Choose a reason for hiding this comment

bb7133 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 7, 2024

ti-chi-bot bot commented Nov 7, 2024

[LGTM Timeline notifier]

cfzjywxk Nov 12, 2024

Choose a reason for hiding this comment

HaoW30 Nov 20, 2024

Choose a reason for hiding this comment

cfzjywxk Nov 12, 2024

Choose a reason for hiding this comment

cfzjywxk Nov 12, 2024

Choose a reason for hiding this comment

wuhuizuo commented Nov 15, 2024

codecov bot commented Nov 20, 2024 • edited Loading

Codecov Report

HaoW30 commented Oct 28, 2024 •

edited by windtalker

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading