RabbitMQ #677
RabbitMQ #677
Conversation
benjben
force-pushed
the
feature/RabbitMQ
branch
2 times, most recently
from
cee1306 to
4d0c1e4
Compare
There was a problem hiding this comment.
That's a quality PR Ben!
One idea worth pondering is whether it generally wouldn't be good to initialise clients outside of Sink/Source resources. Currently there is a nice init flow and the fs2-rabbit is a well-designed library that has a clear algebra-interpreter separation. That really makes it easy to set up a test-harness for all the edge cases around ACK/connection failures etc with the way how we configure it. Probably not in scope of this PR, but an idea for the future maybe.
modules/rabbitmq/src/main/scala/com/snowplowanalytics/snowplow/enrich/rabbitmq/Source.scala
Outdated
Show resolved
Hide resolved
| ssl: Boolean, | ||
| internalQueueSize: Int, | ||
| requestedHeartbeat: Int, | ||
| automaticRecovery: Boolean |
There was a problem hiding this comment.
I spent the most time in this review looking at this topic and still can't work out what we should be doing here by default. What should be our take on this in practice? Should we recover by default?
On one hand, automatic recovery has been a default in rabbitmq clients in many years, yet it comes with its own set of issues. We don't really run dynamic or TTL queues here, so most of the limitations of AQMP don't really count, so it should be safe to use automatic recovery.
One thing that I can't see is fs2-rabbit is publisher confirms - when we publish do we assume transactional ACK from the broker so no messages are lost much like we do with consumer. This comes important when we'd like to do an automatic recovery and the connection drops after sending messages. So we don't really know if they've been ACKed by the broker. I haven't done enough digging on that topic in fs2-rabbit, but it's worth exploring.
I wonder about notifications about recovery attempts. They seem to be logged by the rabbit-client these days, but there's a way for us to also set up our own RecoveryListener if we needed that for visibility/reporting.
There was a problem hiding this comment.
when we publish do we assume transactional ACK from the broker so no messages are lost much like we do with consumer
We ack only after this line has returned successfully. Are you saying that this is possible that this line returns successfully, although the events failed to get published?
There was a problem hiding this comment.
That's what I'm not sure about. When the AMQP goes down and does not do a confirm this line will fail, so when it (automatically?) recovers we publish twice. I think we're fine, but it's worth checking various scenarios in testing.
It would be great indeed ! That's something that I've already considered. I will not do it now because it also impacts |
benjben
force-pushed
the
feature/RabbitMQ
branch
3 times, most recently
from
eff64f0 to
674fa0e
Compare
benjben
force-pushed
the
feature/RabbitMQ
branch
from
674fa0e to
2b13d39
Compare
No description provided.