Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable error response handling (custom nacks) #177

Merged
merged 2 commits into from
Nov 8, 2015

Conversation

erithmetic
Copy link
Contributor

Code related to #165

By default a nack is always sent on an exception.

Allows configuring custom handlers similar to the way logging is configured:

class Requeue
  def handle(delivery_info, properties, broker, e)
    if e.is_a?(Net::HTTPRetriableError) && !delivery_info.redelivered
      broker.requeue delivery_info.delivery_tag
      true
    else
      # Pass and let default nack handler nack the message
      false
    end
  end
end

Hutch::Config.set :error_acknowledgements, [
  Requeue.new
]

@@ -129,11 +130,23 @@ def handle_error(message_id, payload, consumer, ex)
end
end

def acknowledge_error(delivery_info, properties, broker, ex)
acks = error_acknowledgements +
[Hutch::Acknowledgements::NackOnAllFailures.new]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply have a non-empty default for error_acknowledgements?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering my own question: because :error_acknowldgements is not a list, it's a chain of responsibility. Why so? Because double-acking would result in a channel exception and make underlying channel useless.

@michaelklishin
Copy link
Member

The overall idea is sound. However, I find the way you fall back to the default a bit odd. Once that is addressed, I think we can move forward with this. Thank you!

class NackOnAllFailures
include Logging

def handle(delivery_info, properties, broker, ex)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the interface here is a single function, it makes more sense to use call so that functions (blocks, procs, etc) can be used as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch the above. It would lead to inconsistency with error handlers, which use handle and it is way too late to change that.

@michaelklishin michaelklishin merged commit e9e3a1f into ruby-amqp:master Nov 8, 2015
@michaelklishin
Copy link
Member

@dkastner merged with some minor tweaks. Thank you!

@erithmetic
Copy link
Contributor Author

Thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants