GH-11194: Use simple confirms in AmqpOutboundEndpoint - #11205
Conversation
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Looks promising!
Please, consider to add a section into the whats-new.adoc.
You can find example in from the previous one which is now changes-7.0-7.1.adoc
Thanks again!
| simpleConfirmsFlow.getInputChannel() | ||
| .send(new GenericMessage<>("test", Collections.singletonMap("rk", queue.getName()))); | ||
| assertThatThrownBy(() -> simpleConfirmsFlow.getInputChannel() | ||
| .send(new GenericMessage<>("test", Collections.singletonMap("rk", queue.getName())))) |
There was a problem hiding this comment.
Looks like we can have a queue.getName() as a variable to reuse.
As well as the whole message is used twice.
Can you explain also, please, how this test works?
What do we try to prove?
That template.receive() looks similar to the one in the previous test.
Thanks
There was a problem hiding this comment.
Thanks, extracted both. I also dropped the template.receive() assertion: the first send() returning already proves the message was accepted, so it wasn't adding anything here.
What the test proves is that the confirm is actually awaited. Before this change, waitForConfirm with no correlation data skipped the wait, so the broker nack was discarded and both sends succeeded. The test fails without the fix.
It differs from testWithReject in where the exception comes from: here the nack surfaces from waitForConfirmsOrDie() as an AmqpIOException rather than the AmqpException the correlated path builds. I narrowed the assertion to that type to make it explicit.
simpleConfirmsOk passes without the fix too. It's only there to check that wrapping the send in invoke() doesn't break the exchange, routing key and header mapping.
Fixes: spring-projects#11194 The `waitForConfirm` option only took effect when the message carried `CorrelationData`, which requires a connection factory configured for correlated publisher confirms. With `ConfirmType.SIMPLE` there is no correlation data, so the option was silently ignored for single messages, even though the `multiSend` path already used Spring AMQP's scoped `waitForConfirmsOrDie()` for iterable payloads. * Send within a `RabbitTemplate.invoke()` scope and call `waitForConfirmsOrDie()` when `waitForConfirm` is true, there is no correlation data and the connection factory is configured for simple publisher confirms * Warn in `endpointInit()` when `waitForConfirm` is set but the connection factory supports no publisher confirms at all, instead of ignoring the option silently * Correct the `setWaitForConfirm()` Javadoc, the XSD and the reference manual, which stated that a `confirm-correlation-expression` is always required * Add `simpleConfirmsOk()` and `simpleConfirmsWithReject()` to `AmqpOutboundEndpointTests2` Signed-off-by: Jun Cho <ryuu.public@gmail.com>
Signed-off-by: Jun Cho <ryuu.public@gmail.com>
Extract the queue name and the message, drop the `receive()` assertion that the first send already proves, and assert the `AmqpIOException` that the broker nack surfaces instead of the generic `AmqpException`. Signed-off-by: Jun Cho <ryuu.public@gmail.com>
The adapter's scoped send and the advice both open a scoped operation on the same template, and nested scopes are not supported. Also mirror the returned message note from the Javadoc in the XSD. Signed-off-by: Jun Cho <ryuu.public@gmail.com>
|
Hi @artembilan, thanks for the review! Added an AMQP section to |
2 participants
Fixes #11194
waitForConfirmis ignored for single messages withConfirmType.SIMPLE, since it only acts when the message carriesCorrelationData. As you suggested in #11187, the send now runs inside aRabbitTemplate.invoke()scope withwaitForConfirmsOrDie()at the end whenwaitForConfirmis true and there is no correlation data, the same waymultiSenddoes.Two decisions the issue does not cover:
isSimplePublisherConfirms(), soCORRELATEDandNONEfactories keep their current behavior.setMultiSend()andBoundRabbitChannelAdvicealready require the same.endpointInit()warns whenwaitForConfirmis set on a factory with no publisher confirms at all, which was previously ignored without a signal.Documented on the option: a returned message does not fail the send, since the broker acks an unroutable mandatory publish; and the scoped send cannot be combined with a
BoundRabbitChannelAdviceon the sameRabbitTemplate.