Skip to content
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

Documentation change: refactors dead-letter exchange example and its description #134

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fireproofsocks
Copy link

This PR affects documentation only. It is submitted as a fix for issue #133

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Just left a couple of doc comments you'll need to address (too painful via GH edits!) but looks great.

lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Show resolved Hide resolved
Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

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

Don't we only need to declare the exchanges before starting the pipelines? The pipelines can still declare their own queue, can't they?

lib/broadway_rabbitmq/producer.ex Outdated Show resolved Hide resolved
lib/broadway_rabbitmq/producer.ex Show resolved Hide resolved
@fireproofsocks
Copy link
Author

Don't we only need to declare the exchanges before starting the pipelines?

Technically, no, but if my experience is representative (and that of teammates I have witnessed), the setup of the RabbitMQ topology is by far the most confusing aspect of getting this to work. It's not clear which options are read or how, much less exactly when they run (admittedly, amqp may lack examples of this too). Just based on the mental friction/confusion surrounding the options, it's worth having an example that sets up the RabbitMQ instance in a single dedicated place just to demonstrate an alternate approach. "This documentation has too many examples!" said no developer ever.

To your point, another way to demonstrate how to set up a dead-letter exchange/queue would be to declare a separate Broadway pipeline that binds to the dead-letter queue (in addition to the MyPipeline module), and that 2nd pipeline module could create the exchange in its :after_connect function.

I favor doing the setup in its own separate place, however. This stems from my opinion that the current :declare, :bindings, and :after_connect options are not clear and it can create unseen dependencies between pipelines (such as the one that initiated issue #133 in the first place). One common misperception I've seen from several developers using broadway_rabbitmq is thinking that the :bindings options somehow restrict which messages are received by a queue. They don't restrict messages received by a queue: they affect how messages are routed by an exchange. One gotcha here goes like this: you add more and more restrictive bindings and think that your pipeline will be never be sent messages that don't include certain headers. In reality, you have to log into the manager and delete the old (more lax) bindings, otherwise theres always a way around the more restrictive bindings. And people forget that bindings are only relevant when publishing messages to the specific exchange that defined them. If you understand intimately how RabbitMQ works, this wouldn't be confusing, but the way the module options are currently presented tends to foster these kinds of misunderstandings instead of heading them off, so IMO some clarifications would be helpful.

It is a similarly confusing problem when specifying an alternate exchange, and again, it's a bit easier to bring clarity to this if the RabbitMQ setup is done before any Broadway pipelines are even in the picture.

TL;DR: Broadway pipelines are scoped not to an exchange, but to a queue, so any arguments/actions that setup exchanges or bindings don't really belong alongside the queue. Therefore it's more clear/explicit/educational/SRP to include an example where the setup of the entire RabbitMQ instance happens separately in one dedicated place.

Sorry for all the words, but I wanted to make sure my reasoning made sense. Coding is easy; communication is hard.

@whatyouhide
Copy link
Collaborator

This stems from my opinion that the current :declare, :bindings, and :after_connect options are not clear

Can you help clarify them then? 🙃

One common misperception I've seen from several developers using broadway_rabbitmq is thinking that the :bindings options somehow restrict which messages are received by a queue. They don't restrict messages received by a queue: they affect how messages are routed by an exchange.

Yep, 100%, but this is confusion about RabbitMQ works, not really about how broadway_rabbitmq works isn't it? I agree clarifications would be helpful but I don't see how this "fosters" the connections, as again this is really RabbitMQ terminology we're using here.

Coding is easy; communication is hard.

Word!

@fireproofsocks
Copy link
Author

fireproofsocks commented Oct 11, 2024

Can you help clarify them then? 🙃

That is what this PR is trying to do. Remember that the current example on main does not work and it omits a key part of the flow, and everything else aside, this PR is an improvement on that current state.

but this is confusion about RabbitMQ works, not really about how broadway_rabbitmq works isn't it?

No. This is about broadway_rabbitmq. The place where broadway_rabbitmq asks for configuration options to be provided is misleading. The options are misleading because they are included in pipeline configuration and included alongside options that are scoped to a queue. Because broadway_rabbitmq asks for these options, one would assume that they apply to the queue being used by the pipeline, but that is not necessarily the case.

Imagine going to a restaurant, placing an order for a hamburger, and the waiter says "Do you want fries with that?" You answer "yes!". Later, your food arrives, but no fries. "Hey, I ordered fries with my hamburger" you say. But the waiter responds "oh -- no, that's not how things work here. You may not have known that because you haven't eaten here before, but when I prompt you 'do you want fries with that', it actually sometimes applies to another table's order, not yours."

You would find this very strange, right? That's a little like what broadway_rabbitmq is doing by supporting the options that maybe configure your exchange and queue, but maybe they don't, and they don't offer a straightforward way to deal with inter-dependencies like the one between a regular exchange and an alternate exchange. If that's how things are at the restaurant, then maybe the waiter should ask a different question; in the case of this library, maybe an example should be included that shows an alternate way of "ordering your meal."

I understand that the existing options are probably fine for lots of setups (although further clarifications, esp. around bindings would be helpful), and I'm not suggesting they be altered. What I am suggesting via this PR is that an alternative example be included that demonstrates how to provision a more complex setup like what is required for a dead-letter queue. I am confident that showing how to ensure that your RabbitMQ instance is provisioned in a way that is more declarative will reduce confusion and make room for further clarifications.

@whatyouhide
Copy link
Collaborator

No. This is about broadway_rabbitmq. The place where broadway_rabbitmq asks for configuration options to be provided is misleading. The options are misleading because they are included in pipeline configuration and included alongside options that are scoped to a queue. Because broadway_rabbitmq asks for these options, one would assume that they apply to the queue being used by the pipeline, but that is not necessarily the case.

I disagree 🙃 It's not about broadway_rabbitmq in my opinion. The options are all related to the queue that the pipeline uses:

  • :queue
  • :declare (declares the queue used by the pipeline)
  • :bindings (forced to bind the queue in :queue)

I don't recall, maybe we let you do declare: [name: "foo"], queue: "bar", but if we do, we shouldn't and that would be a nice (and welcome!) improvement.

Imagine going to a restaurant, placing an order for a hamburger, and the waiter says "Do you want fries with that?" You answer "yes!". Later, your food arrives, but no fries. "Hey, I ordered fries with my hamburger" you say. But the waiter responds "oh -- no, that's not how things work here. You may not have known that because you haven't eaten here before, but when I prompt you 'do you want fries with that', it actually sometimes applies to another table's order, not yours." You would find this very strange, right?

I don't think the example was necessary.

Btw, I’m all for adding the example, I’m just trying to make sense of the use case 🙃 And sorry for the late reply, been swamped with stuff to do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants