support requeue header on nack by gm-ghanover · Pull Request #102 · stomp-php/stomp-php

gm-ghanover · 2017-11-07T23:09:14Z

as of rabbitmq 3.4, the NACK command supports the requeue header, necessary for DLX

staabm · 2017-11-08T07:56:12Z

     *
     * @param \Stomp\Transport\Frame $frame
     * @param string $transactionId
+     * @param bool $requeue


please describe what this parameter is used for

the requeue header on NACK will either requeue the message (default), or discard the message if false. if DLX is enabled for the queue, requeue=false will send the message to the deadletter exchange. see https://www.rabbitmq.com/dlx.html

Please add a small note to the parameter doc, something like "Parameter are not available on all brokers".

staabm · 2017-11-08T07:58:24Z

DLX ?

jmglsn · 2017-11-08T11:10:03Z

The parameter needs to be part of the other protocol versions too, see failing tests here: https://travis-ci.org/stomp-php/stomp-php/jobs/298826443#L1335.

@staabm We need to check what should happen if this is set on the other protocols, they don't provide this functionality.
I would like to see an LogicException then.
In that case the default value must be null as true would not work.

staabm · 2017-11-08T11:47:15Z

We need to check what should happen if this is set on the other protocols, they don't provide this functionality.

in case the concept of "requeue" does only exist in certain protcols (or in certain msgqueues) we maybe should use a options array or a parameter object instead of a regular parameter.

gm-ghanover · 2017-11-08T15:43:53Z

as far as i know, dead letter queue on activemq is handled via config in activemq.xml http://activemq.apache.org/message-redelivery-and-dlq-handling.html
i haven't tested on activemq, but i did test on older versions of rabbitmq (3.1.*), and the requeue header was ignored, as it wasn't added until 3.4

jmglsn · 2017-11-09T19:57:00Z

@staabm yes, we're ending up in many very specific parameters here. I think for the current version it would be ok just to add a new one. Next major release should reduce this again, I would like to use a parameter object then - it feels like a bigger change and might also reduce the parameters on other protocol functions.
For now I would add LogicException for any protocol that is not capable to cover this parameter. This makes sure that nobody uses the parameter and faces unreported data loss.
@gm-ghanover Thanks for checking this, would you like to add the exception for the other protocol implementations? Otherwise we could append this to your PR.

gm-ghanover · 2017-11-09T22:20:01Z

Older versions of rabbitmq (and i assume activemq) that do not support the requeue header will go with their default behavior and requeue the message on NACK (unless otherwise configured at the service level), so I don't think there is any risk of data loss. The only issue that might arise is someone trying to use requeue=false on something which is not supported, and causing confusion when the message is not discarded (or sent to deadletter).

Throwing a LogicException on activemq could make sense to prevent that, but as far as I'm aware, there is no method for determining the version of rabbitmq that is in use, so there is no method to know if the header is supported or not.

jmglsn

That's a nice change, Thank you for your input!
If you like please add some small tests, otherwise we'll do this.

Please squash and we'll merge :)

jmglsn · 2017-11-10T08:04:45Z

            throw new StompException('Stomp Version 1.0 has no support for NACK Frames.');
        }
        $nack = $this->createFrame('NACK');
+        if ($requeue === false) {


It should be possible to send requeue=>true too, when I read http://www.rabbitmq.com/amqp-0-9-1-reference.html#basic.nack.requeue it feels as if there is an way to configure the server to change the default behavior, not sure if this applies to the stomp plugin http://www.rabbitmq.com/stomp.html too.
But from my point of view it makes sense to allow true and false to be transmitted.
So the condition should be based on $requeue !== null.

jmglsn · 2017-11-10T08:09:56Z

     *
     * @param \Stomp\Transport\Frame $frame
     * @param string $transactionId
+     * @param bool $requeue


Please add a small note to the parameter doc, something like "Parameter are not available on all brokers".

jmglsn · 2017-11-10T08:10:43Z

     * @return void
     */
-    public function nack(Frame $frame)
+    public function nack(Frame $frame, $requeue = null)


Please add the comment from Protocol here too.

staabm · 2017-11-10T08:21:54Z

    {
+        if ($requeue !== null) {
+            throw new \LogicException(
+                'requeue header not supported by ActiveMQ. Please read ActiveMQ DLQ documentation.'


why do you mention the docs here? is ActiveMQ DLQ a euqivalent feature as $requeue?

yes, though configured through activemq.xml, not something you can do individually with message headers.

staabm · 2017-11-10T08:22:36Z

     * Not acknowledge consumption of a message from a subscription
     *
     * @param Frame $frame
+     * @param bool $requeue


here same doc, as requested above.

gm-ghanover · 2017-11-11T02:37:27Z

i'll see what i can do about unit tests this weekend, but i don't have docker set up, so you may beat me to it.

jmglsn · 2017-11-14T08:48:20Z

Please squash your commits :)

staabm · 2017-11-14T08:49:27Z

Please squash your commits :)

@jmglsn in case you dont know. you can squash via github web ui

staabm · 2017-11-14T08:50:51Z

     *
     * @param \Stomp\Transport\Frame $frame
     * @param string $transactionId
+     * @param bool $requeue Requeue header supported on RabbitMQ >= 3.4


shouldn't we implement it in the RabbitMq class then?

that would probably make more sense. should the base protocol then also throw the LogicException if $requeue !== null?

yes, I think so.

default $requeue to null correct getNackFrame to match interface. I assume since Apoolo is built off ActiveMQ, it also doesn't support requeue header. correct grammar in exception message * requeue parameter doc * handle requeue true or false requeue unit tests move requeue header processing to RabbitMQ version check before creating frame

jmglsn · 2017-11-15T07:40:06Z

@staabm Was not sure about who is going to be author of those commits then, do you know that?

staabm · 2017-11-15T08:08:42Z

@jmglsn authorship will stay intact (not sure what happens in case you have commits of different users though).

I know it works when all commits are from the same user.

jmglsn · 2017-11-15T19:37:55Z

@gm-ghanover Thank you! Your change is included in https://github.com/stomp-php/stomp-php/releases/tag/4.3.1

staabm reviewed Nov 8, 2017

View reviewed changes

jmglsn requested changes Nov 10, 2017

View reviewed changes

staabm reviewed Nov 10, 2017

View reviewed changes

gm-ghanover force-pushed the feature/requeue branch from 80146e8 to 6e5ef9b Compare November 10, 2017 23:12

jmglsn approved these changes Nov 14, 2017

View reviewed changes

staabm reviewed Nov 14, 2017

View reviewed changes

gm-ghanover force-pushed the feature/requeue branch from abc8ff4 to 9e2443c Compare November 14, 2017 15:40

gm-ghanover force-pushed the feature/requeue branch from 9e2443c to fc60f4a Compare November 14, 2017 15:43

jmglsn merged commit 8bde4a3 into stomp-php:master Nov 15, 2017

Uh oh!

Uh oh!

Conversation

gm-ghanover commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staabm commented Nov 8, 2017

Uh oh!

jmglsn commented Nov 8, 2017

Uh oh!

staabm commented Nov 8, 2017

Uh oh!

gm-ghanover commented Nov 8, 2017

Uh oh!

jmglsn commented Nov 9, 2017

Uh oh!

gm-ghanover commented Nov 9, 2017

Uh oh!

jmglsn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gm-ghanover commented Nov 11, 2017

Uh oh!

jmglsn commented Nov 14, 2017

Uh oh!

staabm commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmglsn commented Nov 15, 2017

Uh oh!

staabm commented Nov 15, 2017

Uh oh!

jmglsn commented Nov 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants