search

benjamin-hodgson / asynqp

An AMQP library for asyncio
MIT License
84 stars 29 forks source link

Queue should not bind to default exchange #78

Closed txomon closed 8 years ago

txomon commented 8 years ago

To fix #77

txomon commented 8 years ago

Probably I would need to write a test also to assert that the default exchange is being treated as an exception...

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.003%) to 95.851% when pulling cb6f534d95f28d198759ef2a572ec91944d707d6 on txomon:fix/default-exchange-exception into 8f55f2f03cba8aea7c8f1532fa283df3bc50b02e on benjamin-hodgson:master.

txomon commented 8 years ago

Now it should be ready for merging.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.2%) to 96.062% when pulling cf7dcdaabb92c2b919ebd2706409dd214e67452d on txomon:fix/default-exchange-exception into 8f55f2f03cba8aea7c8f1532fa283df3bc50b02e on benjamin-hodgson:master.

benjamin-hodgson commented 8 years ago

As far as I recall, RabbitMQ doesn't allow you to bind a queue to the default exchange. The default exchange will always send the message to the queue named by the routing key. So you shouldn't really be attempting to bind your queue to the default exchange in the first place.

The debate here, therefore, is whether asynqp should allow you to bind a queue to the default exchange (but silently do nothing) or throw an exception. A quick survey of other clients shows that Pika doesn't appear to handle it at all and Kombu silently does nothing.

I have a slight preference for throwing an exception when you bind to the default exchange. Better to make people think about what they're doing. (The existing error message could certainly be improved upon, though.)

txomon commented 8 years ago

Indeed, it doesn't allow you to bind. I took this implementation idea from kombu.

In my case, I would make it transparent, because that way you can use it generically. My code for example, is running in an executor that has different kind of actions builtin, and one of them is queueing a message in an AMQP queue.

Therefore the idea of making the queue bind skip silently. Without this patch, I would have needed to create the QueueBinding manually to avoid the publishing fail, which I am not really in favor of.

tvoinarovskyi commented 8 years ago

Hi, I too am in favor of receiving an Exception upon calling a bind on default exchange. By specification you don't need to do that at all. I think it's better for the user to think 2-ce what he is doing.

txomon commented 8 years ago

@Drizzt1991 This code could be replaced by checking in publish() if the exchange is the default or not, and not raising an exception if there is no queue binding and it's the default exchange.

I am ok with any solution as long as it doesn't mean another call to asynqp, although I still prefer the transparent way because that way you don't need if/else clause to bind or not.

benjamin-hodgson commented 8 years ago

Let's throw an exception from bind then. I'm going to close this PR

txomon commented 8 years ago

@benjamin-hodgson I don't see any update in master that does the exception thing, nor a solution to not generating manually a QueueBinding...

benjamin-hodgson commented 8 years ago

@txomon Apologies, looks like this slipped through the cracks. Fixed in master

txomon commented 8 years ago

No worries, will check how you managed ;)

On Wed, 29 Jun 2016 22:03 Benjamin Hodgson, notifications@github.com wrote:

@txomon https://github.com/txomon Apologies, looks like this slipped through the cracks. Fixed in master https://github.com/benjamin-hodgson/asynqp/commit/59e839f81f4e230370a5a13b4e2c00b07ce73d08

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/benjamin-hodgson/asynqp/pull/78#issuecomment-229488039, or mute the thread https://github.com/notifications/unsubscribe/AAN7mptZT3ouNn8H5YsHDm0KeE_7N9eUks5qQt2YgaJpZM4IQ7I1 .