Skip to content

[Messenger] - Add option to confirm message delivery in Amqp connection #37759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 12, 2020

Conversation

scyzoryck
Copy link

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Hello! My first PR here.

Sometimes we need to be sure that messages are delivered to Amqp. To make it work we need to wait for the confirmation from the Amqp server. So let's wait for it confirmation if confirmation timeout is defined. Default behaviour are not modified - waiting for confirmation will impact performance of sending message.

@scyzoryck scyzoryck requested a review from sroze as a code owner August 6, 2020 20:25
@scyzoryck scyzoryck changed the title Messenger - Add option to confirm message delivery in Amqp connection [Messenger] - Add option to confirm message delivery in Amqp connection Aug 6, 2020
@scyzoryck scyzoryck force-pushed the messenger-amqp-confirm-message branch 3 times, most recently from 6fcb8cd to b542b7f Compare August 10, 2020 21:52
@scyzoryck scyzoryck requested a review from Tobion August 10, 2020 21:54
@nicolas-grekas nicolas-grekas added this to the next milestone Aug 31, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM, with just one minor comment.

@@ -6,3 +6,4 @@ CHANGELOG

* Introduced the AMQP bridge.
* Deprecated use of invalid options
* Add option to confirm message delivery
Copy link
Member

Choose a reason for hiding this comment

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

can you please move this to a new 5.2 section?

Copy link
Author

Choose a reason for hiding this comment

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

Sure! ;-)

@scyzoryck scyzoryck force-pushed the messenger-amqp-confirm-message branch from b542b7f to 7b40ff9 Compare August 31, 2020 21:33
@scyzoryck scyzoryck force-pushed the messenger-amqp-confirm-message branch from 7b40ff9 to 5682d76 Compare August 31, 2020 21:34
@fabpot
Copy link
Member

fabpot commented Sep 12, 2020

Thank you @scyzoryck.

@fabpot fabpot mentioned this pull request Oct 5, 2020
@scyzoryck scyzoryck deleted the messenger-amqp-confirm-message branch November 7, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants