executor: fix the hang of parallel sort when resultChannel is full #59658

xzhangxian1008 · 2025-02-20T02:50:25Z

What problem does this PR solve?

Issue Number: close #59655

Problem Summary:

What changed and how does it work?

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.

Fix the issue that parallel sort will hang when `resultChannel` is full

tiprow · 2025-02-20T02:50:45Z

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

xzhangxian1008 · 2025-02-20T03:05:40Z

/cc @wshwsh12 @XuHuaiyu

codecov · 2025-02-20T03:26:59Z

Codecov Report

Attention: Patch coverage is 92.98246% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.6858%. Comparing base (0ac0618) to head (11963e6).
Report is 92 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #59658        +/-   ##
================================================
+ Coverage   72.9595%   73.6858%   +0.7263%     
================================================
  Files          1694       1705        +11     
  Lines        468654     483235     +14581     
================================================
+ Hits         341928     356076     +14148     
- Misses       105675     105801       +126     
- Partials      21051      21358       +307

Flag	Coverage Δ
integration	`42.9719% <1.7543%> (?)`
unit	`72.5233% <92.9824%> (+0.3749%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.9381% <ø> (+1.8881%)`	⬆️

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

xzhangxian1008 · 2025-02-20T07:24:21Z

/hold

xzhangxian1008 · 2025-02-21T01:25:15Z

/unhold

xzhangxian1008 · 2025-02-21T01:25:43Z

/retest

tiprow · 2025-02-21T01:26:04Z

@xzhangxian1008: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

xzhangxian1008 · 2025-02-21T05:36:07Z

/cc @wshwsh12 @XuHuaiyu

xzhangxian1008 · 2025-02-26T04:46:49Z

/cc @wshwsh12 @XuHuaiyu

xzhangxian1008 · 2025-02-27T06:20:26Z

/cc @wshwsh12 @XuHuaiyu

wshwsh12 · 2025-02-27T06:25:26Z

pkg/executor/sortexec/parallel_sort_spill_helper.go

 	workerWaiter := &sync.WaitGroup{}
 	workerWaiter.Add(workerNum)
 	sortedRowsIters := make([]*chunk.Iterator4Slice, workerNum)
+	errChannel := make(chan error, workerNum*2)


Why workerNum*2?

Why workerNum*2?

One worker may put to err into channel respectively at L145 and L132.

pkg/executor/sortexec/parallel_sort_spill_helper.go

xzhangxian1008 · 2025-03-03T04:23:30Z

/retest

tiprow · 2025-03-03T04:23:53Z

@xzhangxian1008: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

xzhangxian1008 · 2025-03-03T05:37:08Z

/cc @wshwsh12 @XuHuaiyu

wshwsh12

Rest LGTM

pkg/executor/sortexec/parallel_sort_spill_helper.go

xzhangxian1008 · 2025-03-03T08:50:22Z

/cc @XuHuaiyu

xzhangxian1008 · 2025-03-04T01:17:54Z

/cc @XuHuaiyu

xzhangxian1008 · 2025-03-06T02:21:34Z

/cc @XuHuaiyu

pkg/executor/sortexec/parallel_sort_test.go

xzhangxian1008 · 2025-03-07T05:43:00Z

/cc @XuHuaiyu

xzhangxian1008 · 2025-03-10T01:40:28Z

/cc @XuHuaiyu

ti-chi-bot · 2025-03-10T05:52:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wshwsh12, XuHuaiyu

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~~ [XuHuaiyu,wshwsh12]

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 · 2025-03-10T05:52:44Z

[LGTM Timeline notifier]

Timeline:

2025-03-03 06:42:53.656650465 +0000 UTC m=+251086.785570203: ☑️ agreed by wshwsh12.
2025-03-10 05:52:43.049202049 +0000 UTC m=+162318.654730976: ☑️ agreed by XuHuaiyu.

xzhangxian1008 · 2025-03-10T06:23:27Z

/unhold

ti-chi-bot · 2025-03-10T07:32:29Z

In response to a cherrypick label: new pull request created to branch release-8.5: #59980.

…59658) (#59980) close #59655

…ingcap#59658) close pingcap#59655

fix

048e9d9

ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/invalid-title release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 20, 2025

xzhangxian1008 changed the title ~~Fix the hang of parallel sort when resultChannel is full~~ executor: fix the hang of parallel sort when resultChannel is full Feb 20, 2025

ti-chi-bot bot added needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. and removed do-not-merge/invalid-title do-not-merge/needs-triage-completed labels Feb 20, 2025

ti-chi-bot bot requested review from wshwsh12 and XuHuaiyu February 20, 2025 03:05

fix ci

07b80f8

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2025

more fix

05cccad

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 20, 2025

update bazxel

64a0ecc

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2025

wshwsh12 reviewed Feb 27, 2025

View reviewed changes

tweaking

1816c56

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2025

wshwsh12 reviewed Mar 3, 2025

View reviewed changes

pkg/executor/sortexec/parallel_sort_spill_helper.go Outdated Show resolved Hide resolved

tweaking

11963e6

wshwsh12 approved these changes Mar 3, 2025

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 3, 2025

XuHuaiyu reviewed Mar 6, 2025

View reviewed changes

pkg/executor/sortexec/parallel_sort_test.go Show resolved Hide resolved

xzhangxian1008 requested a review from XuHuaiyu March 6, 2025 06:50

XuHuaiyu approved these changes Mar 10, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 10, 2025

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2025

ti-chi-bot bot merged commit 72517f4 into pingcap:master Mar 10, 2025
24 checks passed

ti-chi-bot mentioned this pull request Mar 10, 2025

executor: fix the hang of parallel sort when resultChannel is full (#59658) #59980

Merged

13 tasks

xzhangxian1008 deleted the fix-59655 branch March 11, 2025 00:28

ti-chi-bot bot removed the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Mar 21, 2025

ti-chi-bot bot pushed a commit that referenced this pull request Mar 25, 2025

executor: fix the hang of parallel sort when resultChannel is full (#…

410dbd2

…59658) (#59980) close #59655

zeminzhou pushed a commit to zeminzhou/tidb that referenced this pull request May 6, 2025

executor: fix the hang of parallel sort when resultChannel is full (p…

73524eb

…ingcap#59658) close pingcap#59655

executor: fix the hang of parallel sort when resultChannel is full #59658

executor: fix the hang of parallel sort when resultChannel is full #59658

Uh oh!

Conversation

xzhangxian1008 commented Feb 20, 2025

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

tiprow bot commented Feb 20, 2025

Uh oh!

xzhangxian1008 commented Feb 20, 2025

Uh oh!

codecov bot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xzhangxian1008 commented Feb 20, 2025

Uh oh!

xzhangxian1008 commented Feb 21, 2025

Uh oh!

xzhangxian1008 commented Feb 21, 2025

Uh oh!

tiprow bot commented Feb 21, 2025

Uh oh!

xzhangxian1008 commented Feb 21, 2025

Uh oh!

xzhangxian1008 commented Feb 26, 2025

Uh oh!

xzhangxian1008 commented Feb 27, 2025

Uh oh!

wshwsh12 Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

xzhangxian1008 Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xzhangxian1008 commented Mar 3, 2025

Uh oh!

tiprow bot commented Mar 3, 2025

Uh oh!

xzhangxian1008 commented Mar 3, 2025

Uh oh!

wshwsh12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xzhangxian1008 commented Mar 3, 2025

Uh oh!

xzhangxian1008 commented Mar 4, 2025

Uh oh!

xzhangxian1008 commented Mar 6, 2025

Uh oh!

Uh oh!

xzhangxian1008 commented Mar 7, 2025

Uh oh!

xzhangxian1008 commented Mar 10, 2025

Uh oh!

ti-chi-bot bot commented Mar 10, 2025

Uh oh!

ti-chi-bot bot commented Mar 10, 2025

[LGTM Timeline notifier]

Uh oh!

xzhangxian1008 commented Mar 10, 2025

Uh oh!

Uh oh!

ti-chi-bot commented Mar 10, 2025

Uh oh!

Uh oh!

codecov bot commented Feb 20, 2025 •

edited

Loading