GH-2572 Treat ConfirmType.SIMPLE same as ConfirmType.CORRELATED #2717
Conversation
| @@ -2344,9 +2344,6 @@ public <T> T invoke(OperationsCallback<T> action, @Nullable com.rabbitmq.client. | |||
| if (channel == null) { | |||
| throw new IllegalStateException("Connection returned a null channel"); | |||
| } | |||
| if (!connectionFactory.isPublisherConfirms()) { | |||
There was a problem hiding this comment.
Why remove altogether?
The discussion was about condition improvement:
if (!connectionFactory.isPublisherConfirms() && !connectionFactory.isSimplePublisherConfirms()) {
There was a problem hiding this comment.
Thanks for taking the time to review this!
I went through the discussion and thought the code and came to (maybe faulty) conclusion that closing the channel never makes sense for the invoke method. Especially for me this totally unexpected and not mentioned in the docs anywhere.
Please let me know which way you would like to see this proceed!
Thanks.
There was a problem hiding this comment.
I believe the close is not required when we are in publish-confirm mode.
Therefore that condition and another PR.
Sorry, I don't see anywhere in discussion that we were going to remove this condition altogether.
|
The other fix feel like more legit: #2651. |
| @@ -2344,7 +2344,7 @@ public <T> T invoke(OperationsCallback<T> action, @Nullable com.rabbitmq.client. | |||
| if (channel == null) { | |||
| throw new IllegalStateException("Connection returned a null channel"); | |||
| } | |||
| if (!connectionFactory.isPublisherConfirms()) { | |||
| if (!connectionFactory.isPublisherConfirms() && !connectionFactory.isSimplePublisherConfirms()) { | |||
There was a problem hiding this comment.
Isn't the change in #2651 more precise and accurate?
There was a problem hiding this comment.
Sorry, this was still a bit WIP. Added this change here as well and added also test for this. Hope this covers this completely.
bjoernhaeuser
force-pushed
the
main
branch
3 times, most recently
from
6dbdd48 to
638ff76
Compare
bjoernhaeuser
changed the title
…::isPublisherConfirms
There was a problem hiding this comment.
Looks like there is failing existing test since we have changed condition, I guess:
RabbitTemplateTests > testPublisherConnWithInvokeAndSimpleConfirmations() FAILED
org.opentest4j.AssertionFailedError:
Expecting value to be false but was true
at app//org.springframework.amqp.rabbit.core.RabbitTemplateTests.testPublisherConnWithInvokeAndSimpleConfirmations(RabbitTemplateTests.java:631)
Yeah, thanks! This test does not make sense anymore, removed it :) |
|
I had to revert the fix to our original discussion: 2bb4f29. |
GH-2572 Do not close channel when
ConfirmType.SIMPLEis used for publisher confirmations.See #2571 (discussion) and #2572 (issue) for reference.