ExchangeSenderOp check writer status only if there is data to flush to MPPTunnel by windtalker · Pull Request #9736 · pingcap/tiflash

windtalker · 2024-12-20T02:33:10Z

What problem does this PR solve?

Issue Number: ref #6233

Problem Summary:
Currently, ExchangeSenderSinkOp check the writer status in ExchangeSenderSinkOp::prepareImpl(), so even if there is currently no data to write, the ExchangeSenderSinkOp need to check the writer status, and the pipeline task maybe blocked if the writer is not ready. In this pr, ExchangeSenderSinkOp only check the writer status if it has data to flush to MPPTunnel.

What is changed and how it works?

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

None

windtalker · 2024-12-20T04:01:16Z

/test pull-unit-test

windtalker · 2024-12-20T04:01:25Z

/test pull-integration-test

SeaRise · 2024-12-23T10:37:50Z

dbms/src/Operators/ExchangeSenderSinkOp.cpp

+OperatorStatus ExchangeSenderSinkOp::tryFlush()
+{
+    assert(buffer);
+    auto res = waitForWriter();


Perhaps moving waitForWriter into Writer::write and Writer::flush would be better. This way, even if the tunnel is busy, write can still batch in memory.

You mean if the block's size is small, it will be cached in the writer without writting to tunnel?

yes, if the total block rows is smaller than batch_send_min_limit, MPPTunnel::write would not be call.
https://github.com/pingcap/tiflash/blob/master/dbms/src/Flash/Mpp/BroadcastOrPassThroughWriter.cpp#L103-L109

Good idea, I reimplement this pr by moving waitForWriter to Writer::flush. But waitForWriter is still needed in ExchangeSenderSinkOp::await

gengliqi

LGTM

gengliqi · 2024-12-27T11:53:51Z

dbms/src/Operators/ExchangeSenderSinkOp.cpp

+    if (need_flush)
+    {
+        // the writer must has data to flush
+        assert(writer->hasDataToFlush());


How about using RUNTIME_CHECK?

SeaRise · 2024-12-30T06:19:15Z

/hold

SeaRise

lgtm
comment /unhold to merge pr.

ti-chi-bot · 2024-12-30T06:19:47Z

[LGTM Timeline notifier]

Timeline:

2024-12-27 11:55:43.143647796 +0000 UTC m=+92278.499652335: ☑️ agreed by gengliqi.
2024-12-30 06:19:46.619098267 +0000 UTC m=+331321.975102807: ☑️ agreed by SeaRise.

dbms/src/Operators/ExchangeSenderSinkOp.h

yibin87

Others LGTM

ti-chi-bot · 2024-12-30T06:22:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gengliqi, SeaRise, yibin87

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~~ [SeaRise,gengliqi,yibin87]

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: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2025-01-02T01:55:34Z

/unhold

windtalker · 2025-01-02T02:07:08Z

/hold

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2025-01-02T03:20:25Z

/unhold

…o MPPTunnel (pingcap#9736) ref pingcap#6233 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

ti-chi-bot · 2025-01-05T09:50:53Z

@windtalker: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-test	`3d63e2c`	link	true	`/test pull-integration-test`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

…o MPPTunnel (pingcap#9736) ref pingcap#6233 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

close #10058 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2024

SeaRise reviewed Dec 23, 2024

View reviewed changes

windtalker force-pushed the exchange_sender_op_check_writer_status_only_if_there_is_data_to_write branch from 43283c9 to f2b866d Compare December 24, 2024 03:22

windtalker changed the title ~~ExchangeSenderOp check writer status only if there is data to send~~ ExchangeSenderOp check writer status only if there is data to flush to MPPTunnel Dec 24, 2024

gengliqi approved these changes Dec 27, 2024

View reviewed changes

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

windtalker force-pushed the exchange_sender_op_check_writer_status_only_if_there_is_data_to_write branch from aa254a7 to 3be2940 Compare December 30, 2024 01:50

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 Dec 30, 2024

SeaRise approved these changes Dec 30, 2024

View reviewed changes

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

yibin87 reviewed Dec 30, 2024

View reviewed changes

dbms/src/Operators/ExchangeSenderSinkOp.h Outdated Show resolved Hide resolved

yibin87 approved these changes Dec 30, 2024

View reviewed changes

windtalker force-pushed the exchange_sender_op_check_writer_status_only_if_there_is_data_to_write branch 2 times, most recently from 3d63e2c to 5495435 Compare December 30, 2024 09:48

windtalker added 4 commits January 2, 2025 09:53

save work

d44986a

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix ut

7912f12

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

2b56f8f

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix build

2f9c8aa

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker added 10 commits January 2, 2025 09:53

delete useless code

5acff97

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

rename

273d117

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix ut

26a726e

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

794a200

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

0fdb83f

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

08b430d

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

96513e2

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

17d9745

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

40a8100

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

save work

ebe5a73

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the exchange_sender_op_check_writer_status_only_if_there_is_data_to_write branch from 5495435 to ebe5a73 Compare January 2, 2025 01:54

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 Jan 2, 2025

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 Jan 2, 2025

refine

5dafb1b

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

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 Jan 2, 2025

ti-chi-bot bot merged commit eb4d9ea into pingcap:master Jan 2, 2025

windtalker deleted the exchange_sender_op_check_writer_status_only_if_there_is_data_to_write branch January 2, 2025 03:29

yibin87 pushed a commit to yibin87/tiflash that referenced this pull request Jan 3, 2025

ExchangeSenderOp check writer status only if there is data to flush t…

db70bc6

…o MPPTunnel (pingcap#9736) ref pingcap#6233 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker added a commit to windtalker/tiflash that referenced this pull request Mar 30, 2025

ExchangeSenderOp check writer status only if there is data to flush t…

caae96a

…o MPPTunnel (pingcap#9736) ref pingcap#6233 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker mentioned this pull request Mar 31, 2025

Query running in pipeline model may hang forever in some cases #10058

Closed

windtalker added a commit to windtalker/tiflash that referenced this pull request Mar 31, 2025

ExchangeSenderOp check writer status only if there is data to flush t…

33c5322

…o MPPTunnel (pingcap#9736) ref pingcap#6233 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker mentioned this pull request Mar 31, 2025

cherry pick #9736 to release-8.5 #10059

Merged

12 tasks

ti-chi-bot bot added a commit that referenced this pull request Apr 7, 2025

cherry pick #9736 to release-8.5 (#10059)

4e59807

close #10058 Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com>

Conversation

windtalker commented Dec 20, 2024 • edited 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!

windtalker commented Dec 20, 2024

Uh oh!

windtalker commented Dec 20, 2024

Uh oh!

SeaRise Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

windtalker Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

SeaRise Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

windtalker Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

SeaRise Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

gengliqi left a comment

Choose a reason for hiding this comment

Uh oh!

gengliqi Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

SeaRise commented Dec 30, 2024

Uh oh!

SeaRise left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Dec 30, 2024

[LGTM Timeline notifier]

Uh oh!

Uh oh!

yibin87 left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Dec 30, 2024

Uh oh!

windtalker commented Jan 2, 2025

Uh oh!

windtalker commented Jan 2, 2025

Uh oh!

windtalker commented Jan 2, 2025

Uh oh!

ti-chi-bot bot commented Jan 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

windtalker commented Dec 20, 2024 •

edited

Loading