Maxretry more flexible

[nevesenin]

Hi,

we are currently evaluating if we can switch from our own bunny wrapper gem to sneakers. To make sneakers fit to our existing infrastructure the MaxRetryHandler needs some more configuration options. I'd like to discuss the idea.

Merten

[jondot]

Hi Merten,
Thanks for taking the time to try Sneakers out!. The handlers were designed in a modular way such that you can be sure you can use what ever handlers you'd like with Sneakers.

Generally there are 3 ways people can contribute handlers:

1. Commit to core handlers (in this case, let's just get a thumbs up from one more person using MaxRetry)
2. Add handlers as you see fit to core Sneakers (even if they are variants of existing ones, so you could call this one for example AdvancedMaxRetry)
3. If the handlers you use contain sensitive logic / IP, you can maintain your own repo like a sneakers-handlers or put them into your worker repo in a folder which contains any handler you would like

So what I'm trying to say - any form of a handler contribution is legitimate, so feel free to keep them coming :)

Regarding current pull request - @rud @justin-yesware what do you think?

[rud]

Thank you for the ping @jondot. A few comments:

• The current MaxRetry is indeed quite rigid, and these changes do look like they would fix some of that rigidity. I'd say some integration test scenarios would be in order as this is tricky territory to modify, and I totally understand they're not the first order of business. The current code here looks like it pretty much defaults the the current behaviour, is that correctly understood?
• I'm all for modernizing or cleaning up code style, but I'd personally prefer it as a separate pull-request from semantic changes. I'd suggest using rubocop for consistency, it's quite neat for this sort of cleanup work.

Hope this is helpful.

[nevesenin]

Thanks for the feedback. Yes, the current behavior doesn't change with the defaults. I aggree to not include the style changes and I'll add an integration test scenario. I also like rubocop and I would like to make another pull request for style changes.

[justinmills]

This will change the current behavior I believe. My understanding of this setting is that if it is unset, the routing-key from the originally published message will be used in the retry queue. When the TTL is hit and it goes to the dead-letter exchange (which will then forward it onto the original queue sneakers is subscribing to), it will again pass along the original message's routing key. By setting the value, you're over-writing the original message's routing key, potentially causing the retried message to be different than when it first went through the queue.

I'm not an expert in this area of Rabbit, as we never don't use routing keys, but I suspect this will cause some configurations to not work. I think a test case of this would be to setup your Sneakers worker to use a queue that binds to a topic exchange with a rule to only get messages that have a routing key of "hello". Then publish a message with that routing key, have your worker fail it so it goes through this retry logic, and set the value for this requeue routing key to be "world" and see if you get it again in your worker. If you do, I think a further validation would be to verify that that message you're getting has the routing key value of "hello" and not "world". My suspicion is that you'll never see the retry and if you do, it will have a routing key value of "world".

[justinmills]

I like the additional configuration options this pull has, and I like the direction this is headed, but fear it will break cases that rely on routing key-based queue binding. An integration test should surface that so +1 to that.

[nevesenin]

Here you get the messages with "world" routing key.

[nevesenin]

I'll add a test as soon as possible.

[nevesenin]

Added integration tests.

[justinmills]

I think I would call these workers something more along the lines of how they behave: AlwaysAckWorker, AlwaysRejectWorker, RejectOnceWorker (which is what you have). It may make them more portable across other tests.

[nevesenin]

Good point. Done.

[adis-io]

Sorry for dummy question, but what should I do with messages in dead queue? I used to sidekiq before, so there it's easy to requeue failed jobs.

[nevesenin]

I close this pull request in favor of #205.