Merge lp://staging/~djfroofy/txamqp/txamqp-return-handling into lp://staging/txamqp

Proposed by Drew Smathers
Status: Rejected
Rejected by: Esteve Fernandez
Proposed branch: lp://staging/~djfroofy/txamqp/txamqp-return-handling
Merge into: lp://staging/txamqp
To merge this branch: bzr merge lp://staging/~djfroofy/txamqp/txamqp-return-handling
Reviewer Review Type Date Requested Status
txAMQP Team Pending
Review via email: mp+3767@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Drew Smathers (djfroofy) wrote :

This adds handling for basic.return messages which is optimal for an application I'm developing - we need to know for certain messages if routing failed. Test case is BrokerTests.test_return. I'm open to changing interface, etc.

Revision history for this message
Esteve Fernandez (esteve) wrote :

"This adds handling for basic.return messages which is optimal for an application I'm developing - we need to know for certain messages if routing failed. Test case is BrokerTests.test_return. I'm open to changing interface, etc.
"

Thanks Drew, however there are two things that I don't see clear:

- pythonize. What's the use case of the escape_keywords option? I mean, I know what it does, but what's the purpose?
- set_return_handler. I'm not sure it's very "Twisted-y", it looks like a regular synchronous callback. Could you use a DeferredQueue instead? (just like basic_deliver)

Anyway, thanks for your work!

Revision history for this message
Drew Smathers (djfroofy) wrote :

"Thanks Drew, however there are two things that I don't see clear:

- pythonize. What's the use case of the escape_keywords option? I mean, I know what it does, but what's the purpose?"

`return' is a keyword so on dispatch we induce the method name (in dispatch.py) as basic_return_ instead of "basic_return", which seems silly. I know there are no two python keywords joined with '_' that form yet another python keyword - knock on wood.

"- set_return_handler. I'm not sure it's very "Twisted-y", it looks like a regular synchronous callback. Could you use a DeferredQueue instead? (just like basic_deliver)"

Sure. I can look more into how DeferredQueue is used in txamqp to emulate the same mechanics for basic.return callbacks.

Thanks for the feedback.

Revision history for this message
Drew Smathers (djfroofy) wrote :

Hi Esteve,

I've looked into implementing DeferredQueue for set_return_handler instead of synchronous callbacks and has a few basic questions. For one, from my understanding the AMQClient.queues is specifically for consumer queues and nothing else, right? If I'm right I assume I would need to segregate returns from deliveries.

Secondly, can you provide a minimal example showing what you think is the best API is for handling basic.return. For example something along the lines of the following for basic.consumer to delivery:

  reply = yield channel.basic_consume(queue="test-queue", no_ack=True)
  queue = yield self.client.queue(reply.consumer_tag)
  msg = yield queue.get(timeout=1)

My guess is the interface might put the responsibility on invoking callbacks on the user, so we'd have something like:

  @inlineCallbacks
  def return_checker():
    queue = yield self.client.return_queue()
    (ch, return_msg) = yield queue.get(timeout=1)
    ... do something with message

  checker = LoopingCall(return_checker)
  checker.start(5)

  ...

Then we'd remove set_return_handler?

Please let me know if this is what you're considering.

Thanks.

Revision history for this message
Esteve Fernandez (esteve) wrote :

Hi Drew,

I'm just writing to tell you that I didn't forget about this issue. Sorry for the slow reply, I've been very busy at work :-(

I've been thinking of the best way to support basic.return and will propose (soon, I hope) a branch so you can review it.

BTW, just for curiosity's sake, what broker are you using?

Revision history for this message
Drew Smathers (djfroofy) wrote :

Hi Esteve,

Thanks for the reply. I've been very busy as well, too busy to do much beyond my recommendation for an interface. Handling basic.return is critical for our application as it's the only way of knowing that messages may have been dropped due to host holding a queue going down, etc. - and guaranteed delivery (for certain classes of messages) is an important aspect of the system.

Concerning broker, we're using RabbitMQ.

Revision history for this message
Esteve Fernandez (esteve) wrote :

Hi Drew,

we just added support for basic_return in lp:~txamqpteam/txamqp/support-basic-return Could you have a look at it and see if it works for you?

Thanks!

> Hi Esteve,
>
> Thanks for the reply. I've been very busy as well, too busy to do much beyond
> my recommendation for an interface. Handling basic.return is critical for our
> application as it's the only way of knowing that messages may have been
> dropped due to host holding a queue going down, etc. - and guaranteed delivery
> (for certain classes of messages) is an important aspect of the system.
>
> Concerning broker, we're using RabbitMQ.

Revision history for this message
Esteve Fernandez (esteve) wrote :

This functionality has been merged into the trunk from the lp:~txamqpteam/txamqp/support-basic-return branch.

Unmerged revisions

13. By Drew Smathers <drew@kieru>

add set_return_handler to TwistedDelegate

Subscribers

People subscribed via source and target branches

to status/vote changes: