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

Allow specifying an optional queue namespace #49

Merged
merged 7 commits into from
Jan 13, 2014

Conversation

tjlivesey
Copy link
Contributor

It would be nice to be able to specify a "namespace" for the queue to avoid conflicts and make it easy to identify what the queue is for e.g. prefixing with the name of the service that contains the consumer.

class FailedPaymentConsumer
  include Hutch::Consumer
  queue_namespace "transactions"
  consume 'gc.ps.payment.failed'

  def process(message)
    mark_payment_as_failed(message[:id])
  end
end

# consumer queue name is "transactions:failed_payment_consumer" 

queue_name.gsub(/([^A-Z:])([A-Z])/) { "#{$1}_#{$2}" }.downcase
queue_name.gsub!(/([^A-Z:])([A-Z])/) { "#{$1}_#{$2}" }
queue_name = queue_name.prepend(@_queue_namespace) if @_queue_namespace
return queue_name.downcase
Copy link
Member

Choose a reason for hiding this comment

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

return is not necessary here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a stylistic choice. I tend to prefer using the return statement when I'm using the same variable multiple times. This might not be in keeping with the rest of the codebase though, I agree.

@nelseric
Copy link

Would it be beneficial to allow explicit naming for queues, or include the consumer name, instead of just the routing key? The routing key is dependent upon the publisher, and descriptive of the kind of data being consumed, but does not provide any information into what the queue actually does with this data

@tjlivesey
Copy link
Contributor Author

The queue name actually is currently based on the consumer class name. FailedPaymentConsumer will result in a queue name of failed_payment_consumer (as per examples) which is one of the reasons it may be useful to have a namespace. There may be an email service and a fraud service that both have a FailedPaymentConsumer class which would cause a naming conflict which (I think) would currently just raise an exception when the second worker process is started.

These could be namespaced as email:failed_payment_consumer and fraud:failed_payment_consumer to avoid this.

@michaelklishin
Copy link
Member

This feature makes sense. Note that if you want to separate queues between environments (e.g. staging vs production), you should use vhosts and not namespaced queue names.

@tjlivesey
Copy link
Contributor Author

Yep, I agree, this wasn't really aimed at environment specific namespaces. It would be possible to namespace in a more 'rubyish' way by just namespacing the actual class under a module but this can become problematic in a Rails environment with autoloading.

@hmarr
Copy link
Contributor

hmarr commented Oct 28, 2013

Apologies for the late response on this.

It seems like a global namespace might suit your use case better - e.g. in the fraud service you'd set Hutch::Config.namespace to fraud. Thoughts?

@tjlivesey
Copy link
Contributor Author

I did think that a global namespace is probably more convenient for the situation I described. The above commit adds this but allows both as I think there are probably still situations where a per-consumer namespace may be useful.

@hmarr
Copy link
Contributor

hmarr commented Oct 29, 2013

The global namespace stuff looks great - thanks for that.

I'm still not 100% convinced on consumer-specific namespacing. Perhaps being able to override the queue name would be a better alternative?

@tjlivesey
Copy link
Contributor Author

Overriding queue names may be desirable as class names are not always an obvious identifier when taken out of context e.g. in the RabbitMQ management console. Global namespaces solve my specific problems however, and I can't think of a use case of the top of my head, so you may well be right!

@hmarr
Copy link
Contributor

hmarr commented Nov 7, 2013

I'd love to get this merged. The combination of a global namespace and the ability to override queue names would be the optimal solution, so would you mind removing the consumer-specific namespacing? Happy to merge after than, then we can address the issue of overriding queue names separately.

@tjlivesey
Copy link
Contributor Author

Sorry to be so slow on this one, been really busy. I have removed the queue namespacing in this commit. I guess for overriding the queue name, the most obvious thing would be to have a class method queue_name but obviously this would mean renaming the internal method for retrieving the name. If this isn't desirable then something like set_queue_name that just sets an instance variable that can be checked in the queue_name method.

@michaelklishin
Copy link
Member

@tjlivesey queue_name should be a public API name, the internal one can be easily changed.

@michaelklishin
Copy link
Member

@tjlivesey not to push but we are preparing to cut a new release. If you want this to make in time, please do the queue_name change discussed above and rebase this against master (currently there are conflicts) in the next few days. Thank you!

@tjlivesey
Copy link
Contributor Author

That should do it!

@michaelklishin
Copy link
Member

@tjlivesey thanks, there still seem to be conflicts, can you please merge master in and resolve them, or rebase?

@tjlivesey
Copy link
Contributor Author

@michaelklishin Oops sorry, my forked master was behind, should be OK now.

michaelklishin added a commit that referenced this pull request Jan 13, 2014
Allow specifying an optional queue namespace
@michaelklishin michaelklishin merged commit 89574c8 into ruby-amqp:master Jan 13, 2014
@michaelklishin
Copy link
Member

👍

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

Successfully merging this pull request may close these issues.

4 participants