Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Support publisher confirmations #110

Merged
merged 10 commits into from
Sep 23, 2019
Merged

Support publisher confirmations #110

merged 10 commits into from
Sep 23, 2019

Conversation

mkorszun
Copy link
Collaborator

@mkorszun mkorszun commented Sep 18, 2019

Publisher confirmations, is a mechanism to ensure that a published message actually reached a broker.

How:
Extend publisher configuration with two optional attributes:

  • enable_confirmations - when set to true, confirmations will be activated on the channel during publisher setup + confirmation will be awaited on every publish
  • max_confirmation_wait_time - maximum time in milliseconds, that publisher will wait for a confirmation from a broker before timeouting

Other:

  • Move test publishers to a separate module

Description

Checklist

  • I have added unit tests to cover my changes.
  • I have improved the code quality of this repo. (refactoring, or reduced number of static analyser issues)
  • I have updated the documentation accordingly

**Context:**
[Publisher confirmations](https://www.rabbitmq.com/confirms.html#publisher-confirms), is
a mechanism to ensure that a published message actually reached a broker.

**How:**
Extend, publisher configuration with two optional attributes:
* `activate_confirmations` - when set to `true`, confirmations will be activated on the channel during publisher setup + confirmation will be awaited on every publish
* `max_confirmation_wait_time` - maximum time in seconds, that publisher will wait for a confirmation from a broker before timeouting

**Other:**
* Move test publishers to a separate module
@coveralls
Copy link

coveralls commented Sep 18, 2019

Coverage Status

Coverage increased (+0.9%) to 88.0% when pulling 4f58f97 on publish-with-confirmations into 79e9840 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 87.903% when pulling d2a5921 on publish-with-confirmations into 79e9840 on master.

Copy link
Collaborator

@vorce vorce left a comment

Choose a reason for hiding this comment

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

Looks promising!

lib/publisher.ex Outdated Show resolved Hide resolved
lib/publisher.ex Show resolved Hide resolved
Copy link
Collaborator Author

@mkorszun mkorszun left a comment

Choose a reason for hiding this comment

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

Still need to fix this one failing test I have introduced for triggering confirmation timeouts.

exchange: "gen_rmq_out_exchange",
uri: "amqp://guest:guest@localhost:5672",
app_id: :my_app_id,
activate_confirmations: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be enable_confirmations: true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! It makes me to realize that it is hard to test confirmations :/ Any ideas?

Copy link
Collaborator

@vorce vorce Sep 20, 2019

Choose a reason for hiding this comment

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

Hmm yeah. Only thing I can think of is to use a mock AMQP.Confirm.wait_for_confirms somehow. :\

@mkorszun mkorszun merged commit 3a57e90 into master Sep 23, 2019
@mkorszun mkorszun deleted the publish-with-confirmations branch September 23, 2019 10:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants