Skip to content

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

Merged
ti-chi-bot[bot] merged 13 commits intopingcap:masterfrom
SeaRise:notify_for_join_probe
Sep 14, 2024
Merged

Pipeline: use notify instead of polling for join probe#9081
ti-chi-bot[bot] merged 13 commits intopingcap:masterfrom
SeaRise:notify_for_join_probe

Conversation

@SeaRise
Copy link
Contributor

@SeaRise SeaRise commented May 23, 2024 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #8869

Problem Summary:

What is changed and how it works?

use notify for wait join build/probe finished in pipeline model.
  • add OneTimeFuture to implement join probe's wait for notify.
  • remove HashJoinProbeTransformOp::awaitImpl

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot 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 23, 2024
@SeaRise SeaRise mentioned this pull request May 23, 2024
Closed
8 tasks
@SeaRise SeaRise changed the title DNM: Pipeline: use notify instead of polling for join probe Pipeline: use notify instead of polling for join probe Jun 6, 2024
@SeaRise
Copy link
Contributor Author

SeaRise commented Jun 6, 2024

/cc @windtalker

@ti-chi-bot ti-chi-bot bot requested a review from windtalker June 6, 2024 06:28
windtalker
windtalker reviewed Sep 2, 2024
View reviewed changes
dbms/src/Operators/HashJoinProbeTransformOp.cpp
probe_transform->finalizeProbe();
switchStatus(ProbeStatus::WAIT_PROBE_FINISH);
return OperatorStatus::WAITING;
return OperatorStatus::HAS_OUTPUT;
Copy link
Contributor

@windtalker windtalker Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why original code return WAITING?

Copy link
Contributor Author

@SeaRise SeaRise Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because later, we need to poll in the wait reactor for other task probes to finish before continuing with the restore build.
Now, return to the CPU thread pool to trigger wait-for-notify.

Copy link
Contributor

@windtalker windtalker Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean return to cpu thread pool, if isProbeFinishedForPipeline return false, it will go to wait-for-notify state?

Copy link
Contributor Author

@SeaRise SeaRise Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

@gengliqi gengliqi Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onWaitProbeFinishDone will be called instead of triggering wait-for-notify because finalizeProbe is called.

Copy link
Contributor Author

@SeaRise SeaRise Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onWaitProbeFinishDone will be called instead of triggering wait-for-notify because finalizeProbe is called.

yes, you are right.

@SeaRise SeaRise requested a review from windtalker September 2, 2024 05:18
@SeaRise
Copy link
Contributor Author

SeaRise commented Sep 2, 2024

/cc @gengliqi

@ti-chi-bot ti-chi-bot bot requested a review from gengliqi September 2, 2024 05:19
windtalker
windtalker reviewed Sep 2, 2024
View reviewed changes
dbms/src/Flash/Pipeline/Schedule/Tasks/OneTimeNotifyFuture.h

namespace DB
{
class OneTimeNotifyFuture : public NotifyFuture
Copy link
Contributor

@windtalker windtalker Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add some comments to explain why it is named as OneTimeNotifyFuture

Copy link
Contributor Author

@SeaRise SeaRise Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, comment added.

@SeaRise SeaRise requested a review from windtalker September 2, 2024 08:00
windtalker
windtalker approved these changes Sep 2, 2024
View reviewed changes
Copy link
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 2, 2024
@windtalker
Copy link
Contributor

windtalker commented Sep 11, 2024

/hold

@ti-chi-bot 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 Sep 11, 2024
@windtalker
Copy link
Contributor

windtalker commented Sep 11, 2024

hold since we need to double check the affects of #9072 and #9429

@SeaRise
Copy link
Contributor Author

SeaRise commented Sep 11, 2024

hold since we need to double check the affects of #9072 and #9429

typo: #9424

gengliqi
gengliqi approved these changes Sep 12, 2024
View reviewed changes
dbms/src/Operators/HashJoinProbeTransformOp.cpp
probe_transform->finalizeProbe();
switchStatus(ProbeStatus::WAIT_PROBE_FINISH);
return OperatorStatus::WAITING;
return OperatorStatus::HAS_OUTPUT;
Copy link
Contributor

@gengliqi gengliqi Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onWaitProbeFinishDone will be called instead of triggering wait-for-notify because finalizeProbe is called.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Sep 12, 2024

[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

Details 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 ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 12, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Sep 12, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-09-02 08:13:27.16077711 +0000 UTC m=+259331.678830032: ☑️ agreed by windtalker.
  • 2024-09-12 05:34:09.803509472 +0000 UTC m=+507319.543933433: ☑️ agreed by gengliqi.

@windtalker
Copy link
Contributor

windtalker commented Sep 14, 2024

/hold cancel

@ti-chi-bot 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 Sep 14, 2024
@windtalker
Copy link
Contributor

windtalker commented Sep 14, 2024

/run-all-tests

@ti-chi-bot ti-chi-bot bot merged commit 2f54142 into pingcap:master Sep 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@windtalker windtalker windtalker approved these changes

@gengliqi gengliqi gengliqi approved these changes

Assignees

No one assigned

Labels

approved lgtm 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SeaRise @windtalker @gengliqi