Pipeline: use notify instead of polling for `ExchangeSender` #9072

SeaRise · 2024-05-22T10:53:07Z

What problem does this PR solve?

Issue Number: ref #8869

Problem Summary:

What is changed and how it works?

use waitForWritable() to replace isWritable()

    auto res = writer->waitForWritable();
    switch (res)
    {
    case WaitResult::Ready:
        return OperatorStatus::NEED_INPUT;
    case WaitResult::WaitForPolling:
        return OperatorStatus::WAITING;
    case WaitResult::WaitForNotify:
        return OperatorStatus::WAIT_FOR_NOTIFY;
    }

Check List

Tests

Tested tpch50, no performance regression

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

SeaRise · 2024-05-22T11:04:14Z

/cc @gengliqi @windtalker

SeaRise · 2024-05-22T11:24:55Z

I don't understand why there's always an error saying "Error: found files NOT formatted dbms/src/Flash/Mpp/tests/gtest_mpptunnel.cpp," in pull-integration-test even though I've already formatted it locally.

Signed-off-by: Lloyd-Pottiger <[email protected]>

SeaRise · 2024-05-23T08:33:02Z

I don't understand why there's always an error saying "Error: found files NOT formatted dbms/src/Flash/Mpp/tests/gtest_mpptunnel.cpp," in pull-integration-test even though I've already formatted it locally.

Thanks a lot, @Lloyd-Pottiger .

SeaRise · 2024-06-05T05:02:14Z

/hold

SeaRise · 2024-06-05T05:49:11Z

/hold cancel

windtalker · 2024-07-10T08:49:30Z

dbms/src/Flash/Executor/PipelineExecutorContext.cpp

+        if (likely(dag_context))
+        {
+            // Cancel the tunnel_set here to prevent pipeline tasks waiting in the WAIT_FOR_NOTIFY state from never being notified.
+            if (dag_context->tunnel_set)


The tunnels will be always closed at the destruct of mpp task, is that too late so you need to close it here?

Yes, if a pipeline task is in the wait_for_notify state, and another pipeline task throws an error and cannot terminate the query, then abortMPPTunnel cannot be called, this is a deadlock.

windtalker · 2024-07-10T08:51:06Z

dbms/src/Flash/Mpp/MPPTask.cpp

            abortReceivers();
+            abortQueryExecutor();


why abort receiver first

This is currently useless and is only used to notify exchange_receiver for the same reason as #9072 (comment).

windtalker · 2024-07-25T06:54:21Z

dbms/src/Operators/ExchangeSenderSinkOp.cpp

+    case WaitResult::Ready:
+        return OperatorStatus::NEED_INPUT;
+    case WaitResult::WaitForPolling:
+        return OperatorStatus::WAITING;


if return WAITING, it will still use wait reactor thread to check if it is ready, is my understand right?

yes, you are right.
Here only wait for tunnel ready will return WaitForPolling.

windtalker · 2024-07-25T07:12:14Z

dbms/src/Flash/Executor/PipelineExecutorContext.cpp

+        if (likely(dag_context))
+        {
+            // Cancel the tunnel_set here to prevent pipeline tasks waiting in the WAIT_FOR_NOTIFY state from never being notified.
+            if (dag_context->tunnel_set)


windtalker

LGTM

gengliqi · 2024-08-28T11:40:57Z

dbms/src/Common/GRPCQueue.h

@@ -139,6 +139,9 @@ class GRPCSendQueue

    bool isWritable() const { return send_queue.isWritable(); }

+    void registerPipeReadTask(TaskPtr && task) { send_queue.registerPipeReadTask(std::move(task)); }


Seems these registerPipeReadTasks are not used in this PR?

Yes, they are used in the exchange receiver PR, but not in this PR.
https://github.com/pingcap/tiflash/pull/9073/files

ti-chi-bot · 2024-09-02T05:58:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gengliqi, 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~~ [gengliqi,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-09-02T05:58:19Z

[LGTM Timeline notifier]

Timeline:

2024-07-25 08:03:36.545832468 +0000 UTC m=+1118638.536773933: ☑️ agreed by windtalker.
2024-09-02 05:58:18.643919548 +0000 UTC m=+251223.161972468: ☑️ agreed by gengliqi.

SeaRise · 2024-09-02T05:59:47Z

/merge

ti-chi-bot · 2024-09-02T06:00:03Z

@SeaRise: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

close #9413 Signed-off-by: xufei <[email protected]>

SeaRise added 4 commits May 22, 2024 18:02

add notify_future for mpptunnel

fa590d8

add for writer

62d3add

for exchange sender

e7d8c9b

remove writable

2ffee76

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 22, 2024

SeaRise mentioned this pull request May 22, 2024

Use notify instead of polling in pipeline model #8869

Closed

8 tasks

Merge branch 'master' into notify_for_sender

f76b7bd

ti-chi-bot bot requested review from gengliqi and windtalker May 22, 2024 11:04

try fmt

563573b

refine ut

e1e920c

SeaRise mentioned this pull request May 22, 2024

Pipeline: use notify instead of polling for ExchangeReceiver #9073

Merged

12 tasks

Lloyd-Pottiger and others added 2 commits May 23, 2024 14:33

format

c5655c6

Signed-off-by: Lloyd-Pottiger <[email protected]>

Merge branch 'master' into notify_for_sender

249cfa9

SeaRise added 3 commits May 30, 2024 18:10

Merge branch 'master' into notify_for_sender

d2814b8

Merge branch 'master' into notify_for_sender

d9640df

fix may hang issue

df23506

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 Jun 5, 2024

SeaRise added 2 commits June 5, 2024 13:31

some fix

0c43a11

fix ut

94d954b

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 Jun 5, 2024

SeaRise added 2 commits June 5, 2024 14:00

refine err msg

dd1316f

Merge branch 'master' into notify_for_sender

4ddcf98

windtalker reviewed Jul 10, 2024

View reviewed changes

SeaRise requested a review from windtalker July 10, 2024 09:16

windtalker reviewed Jul 25, 2024

View reviewed changes

SeaRise requested a review from windtalker July 25, 2024 07:14

windtalker approved these changes Jul 25, 2024

View reviewed changes

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

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2024

Merge branch 'master' into notify_for_sender

9c9ffe6

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 26, 2024

gengliqi reviewed Aug 28, 2024

View reviewed changes

SeaRise requested a review from gengliqi August 29, 2024 08:00

Merge branch 'master' into notify_for_sender

f39562e

gengliqi approved these changes Sep 2, 2024

View reviewed changes

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

Merge branch 'master' into notify_for_sender

a328e54

ti-chi-bot bot merged commit 0a80ae0 into pingcap:master Sep 2, 2024
5 checks passed

windtalker mentioned this pull request Sep 6, 2024

tpch query 10 hangs after #9072 #9413

Closed

SeaRise deleted the notify_for_sender branch September 6, 2024 05:28

windtalker mentioned this pull request Sep 9, 2024

Fix TiFlash hang issue after #9072 #9424

Merged

12 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Sep 10, 2024

Fix TiFlash hang issue after #9072 (#9424)

1be6569

close #9413 Signed-off-by: xufei <[email protected]>

windtalker mentioned this pull request Sep 11, 2024

Pipeline: use notify instead of polling for join probe #9081

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline: use notify instead of polling for `ExchangeSender` #9072

Pipeline: use notify instead of polling for `ExchangeSender` #9072

SeaRise commented May 22, 2024 •

edited

Loading

SeaRise commented May 22, 2024

SeaRise commented May 22, 2024 •

edited

Loading

SeaRise commented May 23, 2024

SeaRise commented Jun 5, 2024

SeaRise commented Jun 5, 2024

windtalker Jul 10, 2024

SeaRise Jul 10, 2024

windtalker Jul 25, 2024

windtalker Jul 10, 2024

SeaRise Jul 10, 2024

windtalker Jul 25, 2024

SeaRise Jul 25, 2024

windtalker Jul 25, 2024

windtalker left a comment

gengliqi Aug 28, 2024

SeaRise Aug 29, 2024

ti-chi-bot bot commented Sep 2, 2024

ti-chi-bot bot commented Sep 2, 2024

SeaRise commented Sep 2, 2024

ti-chi-bot bot commented Sep 2, 2024

		@@ -139,6 +139,9 @@ class GRPCSendQueue

		bool isWritable() const { return send_queue.isWritable(); }

		void registerPipeReadTask(TaskPtr && task) { send_queue.registerPipeReadTask(std::move(task)); }

Pipeline: use notify instead of polling for ExchangeSender #9072

Pipeline: use notify instead of polling for ExchangeSender #9072

Conversation

SeaRise commented May 22, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

SeaRise commented May 22, 2024

SeaRise commented May 22, 2024 • edited Loading

SeaRise commented May 23, 2024

SeaRise commented Jun 5, 2024

SeaRise commented Jun 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 2, 2024

ti-chi-bot bot commented Sep 2, 2024

[LGTM Timeline notifier]

SeaRise commented Sep 2, 2024

ti-chi-bot bot commented Sep 2, 2024

Pipeline: use notify instead of polling for `ExchangeSender` #9072

Pipeline: use notify instead of polling for `ExchangeSender` #9072

SeaRise commented May 22, 2024 •

edited

Loading

SeaRise commented May 22, 2024 •

edited

Loading