search

dashbitco / broadway_rabbitmq

A Broadway producer for RabbitMQ
193 stars 42 forks source link

Support custom_pool #118

Closed fcevado closed 1 year ago

fcevado commented 2 years ago

this is a initial approach to tackle #116. opening this one to get some feedback on the actual approach.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5e90b74a221be7d48a59fe8913c7103b871b7bf3-PR-118

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/broadway_rabbitmq/amqp_client.ex 6 31 19.35%
<!-- Total: 6 31 19.35% -->
Totals Coverage Status
Change from base Build 1a9ce143fcdc4b7f5ffa6e51c88e29f55b7b3f86: -4.9%
Covered Lines: 154
Relevant Lines: 217
💛 - Coveralls
fcevado commented 1 year ago

I think this covers the basics. Probably it needs some tests on how it behaves on connecting/reconnecting with the pool, but since the ProducerTest has the client mocked, I guess it would require a test setup with a real broker available to exercise those scenarios.

whatyouhide commented 1 year ago

The approach of having checking and checkout functions looks okay to me, but I'd rather make those a behaviour. So, we'd have an interface for defining pools:

defmodule BroadwayRabbitMQ.ChannelPool do
  @type state() :: term()
  @callback init(args :: term()) :: state()
  @callback checkout_channel(state()) :: {:ok, AMQP.Channel.t()} | {:error, reason :: term()}
  @callback checkin_channel(state()) :: :ok | {:error, reason :: term()}
end

Thoughts on this approach?

fcevado commented 1 year ago

@whatyouhide just a question about the behaviour approach, should we prevent usage of an anymous function then? just accept a module with the proper behaviour implemented

whatyouhide commented 1 year ago

Thanks @fcevado! 💟