MaxRetryHandler tries to set wrong x-dead-letter-exchange value

[dwkoogt]

Looks like this was introduced in version 2.12.0.

I had previously defined retry queues and exchanges as follows: exchanges:

• activejob-retry
• activejob-retry-requeue
• activejob-error

queues:

• activejob-retry
• activejob-error

And from all other queues (lets say these are worker queues), the x-dead-letter-exchange is set to 'activejob-retry'

Now, the MaxRetryHandler has a new convenience method 'configure_queue' that tries to set x-dead-letter-exchange and it is using the queue names instead even though I have 'retry_exchange' configuration option defined.

Thus I get the following error: (lets say the queue is 'mailer')

Unexpected error PRECONDITION_FAILED - inequivalent arg 'x-dead-letter-exchange' for queue 'mailer' in vhost '/': received 'mailer-retry' but current is 'activejob-retry'

I am also passing the x-dead-letter-exchange value as arguments but it is ignored.

class MailerWorker
  include Sneakers::Worker
  from_queue :mailer, { handler: Sneakers::Handlers::Maxretry, retry_routing_key: 'mailer', arguments: { :'x-dead-letter-exchange' => "activejob-retry" } }
end

376 introduced it

427 does not fix it

[dwkoogt]

I realized there is something flawed with the #376 logic when I tried to fix the problem.

In Sneakers, we can configure max_retry_handler by setting retry_exchange, retry_error_exchange, retry_requeue_exchange in the global configuration.

Sneakers.configure ...
  retry_exchange: 'activejob-retry',
  retry_error_exchange: 'activejob-error',
  retry_requeue_exchange: 'activejob-retry-requeue'

The newly defined configure_queue method on line 85 of maxretry.rb tries to retrieve the :retry_exchage.

def self.configure_queue(name, opts)
  retry_name = opts.fetch(:retry_exchange, `"#{name}-retry")
  opts.merge(arguments: { "x-dead-letter-exchange": retry_name })
end

Now the problem part is line 37 of queue.rb where it is passing :queue_options as opt arg to configure_queue method.

if handler_klass.respond_to?(:configure_queue)
    @opts[:queue_options] = handler_klass.configure_queue(@name, @opts[:queue_options])
end

The configure_queue method will look for :retry_exchange value from queue_options instead of global options. So for this to work, one has to configure Sneakers as follows:

Sneakers.configure ...
  retry_exchange: 'activejob-retry',
  retry_error_exchange: 'activejob-error',
  retry_requeue_exchange: 'activejob-retry-requeue'
  queue_options: {
    retry_exchange: 'activejob-retry'
  }

The queue_options we set in global should be just the DEFAULT options. I don't think it should contain any handler specific stuff that belongs in individual workers that implements whatever handlers. The change in #376 also overwrites any queue arguments as well (hence #427 but it doesn't address the main problem).

I think best thing to do is to revert this change. @michaelklishin

[michaelklishin]

Is to revert what change specifically?

Unfortunately reverting it would also be a breaking change. The question at this point should be: how many people would be affected in each case.

[dwkoogt]

The change I'm referring to is the entire #376. I know you have to consider breaking changes but #376 already broke mine and @sharshenov's and we filed issues.

And whoever is using it with #{:queue_name}_retry_xxx convention would have now ignored x-dead-letter-exchange argument living in every one of their workers implementing max retry handler. It will still work for them and some may never notice.

If it didn't break for them, reverting won't break them either.

And having to define retry_exchange value in multiple places is not an ideal solution. I understand what #376 wants to do but It undermines the configurability.

If not reverting, then it should be fixed to not overwrite argument (#427) AND x-dead-letter-exchange value if specified. In any case, it should pass the entire @opts to get correct :retry_exchange value.

[michaelklishin]

Would you be interested in submitting a PR that does that and adds some relevant tests? This is open source software after all.

[dwkoogt]

I had a feeling you were gonna suggest that. I'll have to find some time to do it. When do you plan to do next release?

[michaelklishin]

As soon as we have this PR in :) we don't do calendar-based releases FWIW.