dashbitco / broadway_rabbitmq

A Broadway producer for RabbitMQ
193 stars 42 forks source link

Allow usage of connection/channel pool #116

Closed fcevado closed 1 year ago

fcevado commented 2 years ago

this relates with the discussion on #60 but i'm creating a new issue to address just the points related to handling the resource that the producer uses to connect/communicate with rabbitmq. just stating some things about rabbitmq and how AMQP lib model stuff:

With that in mind, what do you think about adding an option pool to the producer config, that option accept a 1-arity function or a mfa. Once a channel is needed we checkout a channel by calling the function passing the producer pid for the pool manager.

josevalim commented 2 years ago

If you can encapsulate all of the operations, mentioned above, through the channel, then I don't see an issue with this approach. Although note that you will also need an option to ask this lib to not start its own pool (or we change the option name to be custom_pool to make it clearer).

fcevado commented 2 years ago

@josevalim I'll work on that, just to define the default behavior. The precedence is in favor of connection option so, if connection is nil and custom_pool is present, the producer try to use the custom_pool.

whatyouhide commented 2 years ago

@fcevado are AMQP channels linked to the connection they belong to? #lazyweb

In any case, you'll also have to handle retries when checking out a channel out of an existing pool of connections, and the AMQP lib makes it quite error-prone to do operations without crashing since you'll need to catch left and right 😄

fcevado commented 2 years ago

@whatyouhide they are linked, but the connection trap exit for known reasons. if the reason a channel dies is caused by amqp operations the connection won't die, but if you send a exit signal to the channel pid, it will die. anyway, this wouldn't be responsability of broadway_rabbitmq. I guess the point here is that the broadway producer just needs a channel to do it's job. if that channel dies, it asks for a new one, the pool manager should be able to handle that correctly. I'm not suggesting that brodway_rabbitmq provides a pool manager.

What worries me is that even if the backoff strategy is stop the broadway pipeline won't die although the producer won't try to restart, I'm inclined to think that this is a broadway behavior in general, not related to broadway_rabbitmq. i need to test it with a custom producer to make sure.

fcevado commented 1 year ago

118 is ready for review. not sure who I should tag for it.

cc @josevalim @whatyouhide

whatyouhide commented 1 year ago

Closed in #118. Thanks @fcevado! 💟