Skip to content

Conversation

@garyrussell
Copy link
Contributor

Support DLQ - the listener must throw an AmqpRejectAndDontRequeueException
and this will send a basic.reject with requeue false (instead of the
default true).

There is an issue with Rabbit DLQ handling with durable queues so,
currently, this only works when dead-lettering messages sent to
non-durable queues.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably do this (a bit shorter)

while (t != null && shouldRequeue){
    shouldRequeue = !(t instanceof AmqpRejectAndDontRequeueException);
    t = t.getCause();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could, but I like mine better - and it's more efficient :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, the "efficiency" comes in the form of not calling 't = t.getCause();' after 'break' ;) Well, I tried ;) but in all seriousness I am just not a big fan of 'breaks', they have to much of a 'go to' smell ;)
Anyway, i'll merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; don't merge - working on James' comment (and it will include your suggestion because of that change :-)

@jamescarr
Copy link
Contributor

Looks good. My only other suggestion is for purists like me who like keep consumers isolated from AMQP is to provide some convenience mechanism in SimpleMessageListenerContainer like setRejectAndRequeue(boolean requeue) so I can define this in my configuration. :)

@garyrussell
Copy link
Contributor Author

Good point - I think we need to retain the current mechanism, so the application can choose the behavior on a message-by-message basis, but the ability to set a global default would be a good thing.

Will rework.

If doStart() was called multiple times, multiple threads ran
in each consumer.

There is a check to prevent creating multiple consumers in
this case, but the thread management had no such check.
LongString moved from client.impl to client

Channel.setReturnListener changed to Channel.addReturnListener
Support DLQ - the listener must throw an AmqpRejectAndDontRequeueException
and this will send a basic.reject with requeue false (instead of the
default true).

There is an issue with Rabbit DLQ handling with durable queues so,
currently, this only works when dead-lettering messages sent to
non-durable queues.

DLQ Polishing - Add Unit Tests

DLQ Polish Tests
Previously, the default requeue behavior was true, with the
ability to override for a specific message by throwing an
AmqpRejectAndDontRequeueException.

However, as posted in a PR review, it is desirable to set
the default behavior using a property on the listener
container.

Now, by setting requeueRejected to false (default=true),
the default becomes false.

When true, the default can still be overriden for a specific
message as described above.
@garyrussell
Copy link
Contributor Author

Configuration option added. Had to rebase to pull in the AMQP-223 fix.

And the while loop should now be more to @olegz 's liking.

@james-carr
Copy link

👍

@jamescarr
Copy link
Contributor

Hey is it possible to publish a snapshot release once this is included?

olegz added a commit to olegz/spring-amqp that referenced this pull request Apr 9, 2012
* AMQP-221:
  AMQP-221 Support Dead Letter Queues
@olegz olegz closed this Apr 9, 2012
@garyrussell
Copy link
Contributor Author

James, the commit is in master, it should make it into tonight's snapshot.

@jamescarr
Copy link
Contributor

Thanks!

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