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.

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 😜

thedrow added Component: RabbitMQ Broker PR Type: Feature labels Aug 25, 2024

thedrow added this to the 5.5 milestone Aug 25, 2024

thedrow requested a review from Nusnus August 25, 2024 08:49

thedrow self-assigned this Aug 25, 2024

thedrow marked this pull request as draft August 25, 2024 08:49

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

thedrow force-pushed the blm-375 branch from 2490bac to 4b5082e Compare September 2, 2024 10:44

thedrow force-pushed the blm-375 branch from 4995546 to 2b8b4a7 Compare September 10, 2024 13:18

thedrow force-pushed the blm-375 branch from 2b8b4a7 to 75771d5 Compare September 11, 2024 15:06

thedrow force-pushed the blm-375 branch 3 times, most recently from 0ae3885 to 7af9d39 Compare September 18, 2024 13:21

thedrow marked this pull request as ready for review September 18, 2024 13:22

Nusnus requested changes Sep 18, 2024

View reviewed changes

Comment thread celery/app/base.py Outdated

Comment thread celery/worker/consumer/delayed_delivery.py

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

thedrow force-pushed the blm-375 branch from 0c3ac06 to 69244bb Compare October 13, 2024 14:50

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

4 tasks

Nusnus requested changes Oct 16, 2024

View reviewed changes

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

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

thedrow force-pushed the blm-375 branch from 42a129e to ff9825d Compare November 17, 2024 11:07

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

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

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

This was referenced Nov 19, 2024

Document usage of broker_native_delayed_delivery_queue_type #9419

Merged

Adjust section in what's new document regarding quorum queues support #9420

Merged

thedrow deleted the blm-375 branch November 25, 2024 13:23

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

Uh oh!

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!

Nusnus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thedrow commented Aug 25, 2024 •

edited

Loading

codecov Bot commented Aug 25, 2024 •

edited

Loading