Review RabbitMQ module

[jaguililla]

Check if connection checks are still needed (with last driver version)

[donky106]

Hi, I would like to work on this issue

[jaguililla]

Ok, I will assign it to you.

The bottom line is that a lot of the code added to check connection errors and assure messages are handled and acknowledged is old (coming from Java 5+ years ago).

It would be great if it could be reduced (removing not needed code) by taking advantage of the latest AMQP Java Client (if possible).

This issue is a bit vague, sorry about that :( If you want to do a simple analysis and see how it could be improved to be smaller and simpler while still being reliable (this is the most important thing for me when dealing with messages), it would be great!

Thanks for your interest, and for your help :)

[donky106]

Thanks for assigning, I appreciate it very much. I will keep you updated

[donky106]

Hi Juanjo,

1. What I noticed you open a new channel for each message and that is what they don't want. Channels supposed to be long-lived.

"A classic anti-pattern to be avoided is opening a channel for each published message. Channels are supposed to be reasonably long-lived and opening a new one is a network round-trip which makes this pattern extremely inefficient. Just like connections, channels are meant to be long-lived. Opening a new channel for every operation would be highly inefficient is highly discouraged. Channels, however, can have a shorter life span than connections. For example, certain protocol errors will automatically close channels. If applications can recover from them, they can open a new channel and retry the operation."

This approach problems when you delete the queue and you did't call chanell.basicCancel() before

2. you do retry() on the channel.basicAck(envelope.deliveryTag, false) in the Handler.kt. I am not sure if you really need it. The protocol states

"If a consumer dies without sending an ack, RabbitMQ will understand a message wasn't processed fully and will redeliver it to another consumer. That way you can be sure that no message is lost, even if the workers occasionally die."

3. you use sleep() but it is not a good idea. There are better ways to do that.

4. you set
   factory.setAutomaticRecoveryEnabled(true);
   factory.setNetworkRecoveryInterval(xxxxx);

so you don't really need to restore the connection manually.

"If recovery fails due to an exception (e.g. RabbitMQ node is still not reachable), it will be retried after a fixed time interval (default is 5 seconds). The interval can be configured." RabbitMQ Java client 4.0 and later versions have automatic connection recovery enabled by default.

I have done very tiny approvements in the code, just have a look. But there is more to be improved.

Let me know please what you think about this.

Thanks a lot

[jaguililla]

Thanks for digging in the documentation and exposing those flaws, it is a valuable information.

I encourage you to make a PR with the improvements you made, and if you feel it could be further improved and want to give it a go, it would be great!

I really appreciate the time you took to improve this feature, thanks again!

[jaguililla]

I've merged your PR. Thanks! let me know if you want to make further improvements to the module :)

[donky106]

Thanks! Working on improvements. Want to build in some metrics too. Will keep you updated.