Native Delayed Delivery in RabbitMQ by thedrow · Pull Request #9207 · celery/celery

thedrow · 2024-08-25T08:49:13Z

Note: Before submitting this pull request, please review our contributing
guidelines.

Description

Fixes #9149.
Since ETA tasks will block the worker when using quorum queues we had to provide an alternative solution that will support ETA tasks on RabbitMQ.
This PR provides a solution based on NServiceBus' implementation of native delayed delivery.
The solution is based on a series of exchanges and queues as detailed in NServiceBus' documentation.
To use this feature you need to enable the broker_native_delayed_delivery configuration setting when using quorum queues.

codecov · 2024-08-25T08:53:40Z

Codecov Report

Attention: Patch coverage is 86.79245% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.15%. Comparing base (fadc1ae) to head (5fdf91e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
celery/app/base.py	83.33%	3 Missing ⚠️
celery/utils/quorum_queues.py	80.00%	2 Missing ⚠️
celery/worker/consumer/delayed_delivery.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9207   +/-   ##
=======================================
  Coverage   78.15%   78.15%           
=======================================
  Files         151      153    +2     
  Lines       18984    19020   +36     
  Branches     2508     2516    +8     
=======================================
+ Hits        14837    14866   +29     
- Misses       3860     3866    +6     
- Partials      287      288    +1

Flag	Coverage Δ
unittests	`78.13% <86.79%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thedrow · 2024-09-11T13:35:06Z

@Nusnus Any idea why the quorum queues smoke tests are failing because the RabbitMQ container is not ready yet?

Nusnus · 2024-09-11T14:19:03Z

@Nusnus Any idea why the quorum queues smoke tests are failing because the RabbitMQ container is not ready yet?

Does it pass locally?

thedrow · 2024-09-11T15:25:53Z

@Nusnus Any idea why the quorum queues smoke tests are failing because the RabbitMQ container is not ready yet?

Does it pass locally?

No it does not. I haven't done anything that can destabilize the test suite so I'm not sure what could be wrong.

Nusnus

Optional comment: Add assertion error messages.
The only reason I bring this up is due to the nature of this change, which is relying on an algorithm to implement an ETA mechanism, so these errors could be crucial, and as the author of the changes, you will probably come up with the best error messages as you understand the protocol.

Food for thought.

Except that, please see my other comments. Something needs fixing too.

Nusnus · 2024-09-18T14:21:12Z

Also please make sure the CI fully passes 🙏

thedrow · 2024-10-13T16:33:08Z

CI fails because I'm installing Kombu from Git. @Nusnus Once you release Kombu's RC I can change the requirements to that RC.

Nusnus

@thedrow

CI fails because I'm installing Kombu from Git. @Nusnus Once you release Kombu's RC I can change the requirements to that RC.

Done. Please rebase on main.
The requirements are already updated.

Ping me if the CI fully passes for a review.
There have been many changes since my last review.

I'll do it today if the CI is green of course.

thedrow · 2024-10-15T16:32:21Z

@Nusnus Ping.

Nusnus

I have done a deep dive into the PR and found some issues.
I found at least 1 bug, some missing doc, cosmetic issues and some questions to make sure I fully understand the code, and some more minor stuff..

@thedrow please review my comments carefully and feel free to ask for clarifications.
I want this finished fast so we can include it in the next RC for Celery, so I’ll give you top priority after everything is fixed/answered.

Lastly, please do not dismiss/resolve my comments and let me review your response/fix to make sure there aren’t any gaps by resolving my own PR comments. We, the maintainers, need to show an example of work methodologies for the rest of the community, so I kindly ask you to join me in maintaining high standards.

Thanks!

Jean-Daniel · 2024-10-16T11:12:44Z

Out of curiosity, what are the benefit of the NServiceBus implementation versus using per-message TTL by specifying the expiration field when sending a delayed message ?

Nusnus

@thedrow

CI fails because I'm installing Kombu from Git. @Nusnus Once you release Kombu's RC I can change the requirements to that RC.

Please take note the new Kombu RC release, which is a requirement for this PR, has introduced a new breaking bug as described in celery/kombu#2157.
Obviously, we do not want to revert any changes, but if we merge this PR without fixing Kombu (first IMHO), then we’ll introduce a dependency in Celery v5.5 to a confirmed broken (RC) version of Kombu.

Please review the Kombu’s report by @woutdenolf in addition to the PR comments from yesterday, as per my explanation above, it is closely related.

thedrow · 2024-10-16T12:52:34Z

Out of curiosity, what are the benefit of the NServiceBus implementation versus using per-message TTL by specifying the expiration field when sending a delayed message ?

I haven't considered other implementations. This just works so there's no need to find other solutions.

Nusnus

Replied to what was answered so far.
Please review my new comments and the rest of yesterday's comments, which are still awaiting your response/fix.

Ping me when you’re done.

P.S
Merge/Rebase on main to make sure you’re up to date.

Nusnus

Reviewed again. Don’t forget to rebase on main.
See new comments.

thedrow · 2024-11-13T12:44:20Z

@Nusnus I addressed most of your concerns excluding the bug which I don't know how to fix.
Please re-review.

thedrow · 2024-11-13T12:53:10Z

The tests are failing because of codecov. I don't know why.
See here.

Nusnus · 2024-11-16T17:32:17Z

The tests are failing because of codecov. I don't know why.
See here.

Rebase on main, there's a new codecov version.

Nusnus · 2024-11-16T17:35:28Z

@Nusnus I addressed most of your concerns excluding the bug which I don't know how to fix.
Please re-review.

Thanks! 🙏
I'll review it again tomorrow/next day!

…ke tests.

This reverts commit ce31560.

…cted.

Nusnus

Well done @thedrow 👏
Thank you for attending to all of the review issues during the work on this new core mechanism in Celery. The community is going to appreciate it a lot, trust me bro 😜

auvipy self-requested a review August 26, 2024 05:59

Nusnus requested changes Sep 18, 2024

View reviewed changes

Comment thread celery/app/base.py Resolved

Comment thread celery/worker/consumer/delayed_delivery.py Resolved

Comment thread celery/worker/consumer/tasks.py Resolved

Nusnus requested changes Oct 14, 2024

View reviewed changes

Nusnus requested changes Oct 15, 2024

View reviewed changes

Nusnus mentioned this pull request Oct 15, 2024

support for quorum queues #6067

Closed

3 of 4 tasks

Nusnus requested changes Oct 16, 2024

View reviewed changes

Nusnus reviewed Oct 16, 2024

View reviewed changes

Comment thread celery/worker/consumer/delayed_delivery.py Resolved

Nusnus mentioned this pull request Oct 17, 2024

Celery v5.5.0 Release #9140

Closed

3 of 6 tasks

Nusnus requested changes Oct 29, 2024

View reviewed changes

Omer Katz added 7 commits November 17, 2024 13:07

Add the DelayedDelivery bootstep.

b70eef4

Comment POC code only

56a182a

Add feature flag.

0eb6d48

Add coverage for include_if.

3681c0e

Remove POC only code.

7f9760d

Added unit tests for delayed delivery implementation.

ad420da

Autopep8.

7530c12

Omer Katz added 25 commits November 17, 2024 13:07

Add documentation regarding native delayed delivery.

94d3ce3

Mention that confirmed publish must be set to true.

a686954

Cover both values of broker_native_delayed_delivery_queue_type in smo…

760f4ee

…ke tests.

Revert usage of broker_native_delayed_delivery_queue_type.

9e85f07

logger.warn is deprecated

6223344

Fix include_if condition to take failover into consideration.

3f2d029

Fix smoke tests.

f4a8da0

Revert "Revert usage of broker_native_delayed_delivery_queue_type."

2e3a32b

This reverts commit ce31560.

Apply x-dead-letter-strategy only on quorum queues.

bcb858c

Fix unit tests.

d15ab53

Use kombu native delayed delivery API.

b0323ac

Add documentation.

c45e9a9

Delayed native delivery queues can now be non-quorum queues.

8f427df

Declare native delayed delivery queues on failover brokers as well.

65ef4b0

Fix unit tests.

ef48659

Use connection to check for the transport type.

792b84a

Add versionadded to the documentation.

560d3f3

Add link to quorum queues migration guide.

b69379e

Fix failover when connection is refused.

56ba9e0

Change native delayed delivery queue type default to quorum.

f518b11

Remove warning.

6e80f37

Use native delayed delivery automatically when quorum queues are dete…

f566dbc

…cted.

Remove the broker_native_delayed_delivery configuration setting.

0768268

Use fixtures and extract common test code.

ff9825d

Adjust documentation.

5fdf91e

auvipy approved these changes Nov 17, 2024

View reviewed changes

Nusnus approved these changes Nov 17, 2024

View reviewed changes

Comment thread celery/worker/consumer/delayed_delivery.py Resolved

Comment thread docs/getting-started/backends-and-brokers/rabbitmq.rst Resolved

Nusnus merged commit fa5d7ff into celery:main Nov 17, 2024

Nusnus mentioned this pull request Mar 9, 2025

Refactored and Enhanced DelayedDelivery bootstep #9599

Merged

froblesmartin mentioned this pull request Dec 3, 2025

Improved ETA support for quorum queues #9149

Closed

Conversation

thedrow commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov Bot commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thedrow commented Sep 11, 2024

Uh oh!

Nusnus commented Sep 11, 2024

Uh oh!

thedrow commented Sep 11, 2024

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nusnus commented Sep 18, 2024

Uh oh!

thedrow commented Oct 13, 2024

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

thedrow commented Oct 15, 2024

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jean-Daniel commented Oct 16, 2024

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

thedrow commented Oct 16, 2024

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thedrow commented Nov 13, 2024

Uh oh!

thedrow commented Nov 13, 2024

Uh oh!

Nusnus commented Nov 16, 2024

Uh oh!

Nusnus commented Nov 16, 2024

thedrow commented Aug 25, 2024 •

edited

Loading

codecov Bot commented Aug 25, 2024 •

edited

Loading