-
Notifications
You must be signed in to change notification settings - Fork 40.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add confirm-type property to RabbitProperties #17848
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I am a bit confused about this new property so I've asked @garyrussell for some help.
/** | ||
* Whether to enable simple publisher confirms. | ||
*/ | ||
private boolean simplePublisherConfirms; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyrussell could you please educate me on the difference between publisherConfirms
and simplePublisherConfirms
. Looking at those two properties, this feels a bit odd to me.
Perhaps we should refactor the configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snicoll With standard publisher confirms, the caller gets a callback for each sent message, together with correlation data so he can figure out which sent message the confirmation is for.
Let's say we send 10 messages back-to-back; the broker might send us 10 discrete confirmations or one confirmation that says "the last 10 sends were good", depending on timing.
In the latter case, the framework still invokes the callback 10 times.
With simple confirms (added in spring-amqp 2.1), the caller is saying "I don't care to be told which messages succeeded or failed, I just want to wait until they all succeed"; the amap-client has a method waitForConfirmsOrDie
which is called by the RabbitTemplate
method with the same name.
See Scoped Operations in the reference guide.
You can still use simple confirms without it, but setting this property removes the additional scaffolding needed to support standard confirms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@garyrussell and I had a little chat offline and I shared having two properties for the same feature was a bit odd. Gary said he'd add an enum in Spring AMQP instead so we'll have to rework this PR to use that.
Wondering if switching a property's type in a feature release is acceptable so flagging for team attention.
Blocked by spring-projects/spring-amqp#1067 |
New enum is in 2.2.0.BUILD-SNAPSHOT spring-projects/spring-amqp@acb5683 |
@htztomic do you have time to update your proposal. Based on AMQP snapshots, the idea is to introduce a new We don't rely on snapshots for AMQP yet but the plan is to do that soon as part of #17898 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is blocked again as upgrading to the snapshots broke our build. I think there is a glitch in the fix on the AMQP side, see spring-projects/spring-amqp#1067 (comment) |
This is a fix to GitHub issue #17828. Additional property simplePublisherConfirms was added to RabbitProperties.