RabbitMQ requeue with additional nack metadata

[michaelkoetter]

Supports specifying the 'requeue' flag using additional nack metadata. Should fix #2200 .

[cescoffier]

LGTM

It needs a bit of documentation.
Also, did you try accessing the metadata from the message itself instead of passing it around?

[michaelkoetter]

It needs a bit of documentation.

You mean in the markdown docs?

Also, did you try accessing the metadata from the message itself instead of passing it around?

I figured the purpose of nack(Throwable reason, Metadata metadata) was to pass metadata that's relevant for nack'ing the message. From javadoc:

Additional metadata may be provided that the connector can use when nacking the message. The interpretation of metadata is connector-specific.

Sure we could simply add the additional metadata to the message, but then what's the point in having that extra signature for nack 😄

[michaelkoetter]

This seems to be tricky. I tried adding the metadata like this

input.addMetadata(new RabbitMQRejectMetadata(true))
                        .nack(new RuntimeException("requeue"));

but then the rejectMessage method doesn't see the metadata. I think this is because an instance of the message is captured in a nack lambda at some earlier point, at least debugging seems to indicate this.

Still trying to figure it out 😄

[ozangunalp]

Thanks for this! It looks good to me too. It'd be great to add some explanation to the receiving-messages-from-rabbitmq.md#failure-management

On the metadata stuff, we've been kind of entertaining the idea of reworking the message composability and maybe deprecating that method nack(Throwable, Metadata).

The current issue is if the message is intercepted in a way to add metadata etc. nack(Throwable, Metadata) method wasn't really composable, and passed metadata would be completely ignored.

It turns out you've discovered that it is not that easy there are already other problems to achieve this.

[michaelkoetter]

@ozangunalp Thanks, I'll add a few sentences to that doc section.

I don't think it's possible to use metadata from the message itself without additional changes. The withMetadata() and addMetadata() methods create a new message instance with references to the original message's ack and nack methods. So in the end, we will always call IncomingRabbitMQMessage.nack() on the original message (which doesn't contain the added metadata)
Also, as metadata is immutable, there is no other way to add metadata to the message.

So should we stick to nack(Throwable, Metadata) or is it a dead end?

[codecov-commenter]

Codecov Report

Merging #2239 (be124f3) into main (3bdc102) will decrease coverage by 66.40%.
The diff coverage is 0.00%.

❗ Current head be124f3 differs from pull request most recent head 15702ef. Consider uploading reports for the commit 15702ef to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #2239       +/-   ##
=============================================
- Coverage     77.95%   11.55%   -66.40%     
+ Complexity     3712      459     -3253     
=============================================
  Files           302      304        +2     
  Lines         12419    12443       +24     
  Branches       1592     1592               
=============================================
- Hits           9681     1438     -8243     
- Misses         2015    10819     +8804     
+ Partials        723      186      -537     

Files Changed	Coverage Δ
.../reactive/messaging/rabbitmq/ConnectionHolder.java	0.00% <0.00%> (-76.09%)	⬇️
...ve/messaging/rabbitmq/IncomingRabbitMQMessage.java	0.00% <0.00%> (-73.44%)	⬇️
...reactive/messaging/rabbitmq/RabbitMQConnector.java	0.00% <ø> (-87.17%)	⬇️
...ive/messaging/rabbitmq/RabbitMQRejectMetadata.java	0.00% <0.00%> (ø)
...ctive/messaging/rabbitmq/fault/RabbitMQAccept.java	0.00% <ø> (-100.00%)	⬇️
...ive/messaging/rabbitmq/fault/RabbitMQFailStop.java	0.00% <0.00%> (-100.00%)	⬇️
...ctive/messaging/rabbitmq/fault/RabbitMQReject.java	0.00% <0.00%> (-100.00%)	⬇️
...tive/messaging/rabbitmq/fault/RabbitMQRequeue.java	0.00% <0.00%> (ø)

... and 249 files with indirect coverage changes

[ozangunalp]

I am working on a prototype to allow this and improve the composability of Messages. I think we'll need to keep nack(Throwable, Metadata) anyways.

[ozangunalp]

Rebased to main, reviewed the test case, added new failure strategy named requeue and added documentation

[cescoffier]

It seems that the CI is unhappy.