Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

htztomic
Copy link
Contributor

This is a fix to GitHub issue #17828. Additional property simplePublisherConfirms was added to RabbitProperties.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 13, 2019
Copy link
Member

@snicoll snicoll left a 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;
Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Member

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.

@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review status: blocked An issue that's blocked on an external project change labels Aug 13, 2019
@snicoll
Copy link
Member

snicoll commented Aug 13, 2019

Blocked by spring-projects/spring-amqp#1067

@wilkinsona wilkinsona removed the for: team-attention An issue we'd like other members of the team to review label Aug 14, 2019
@garyrussell
Copy link
Contributor

New enum is in 2.2.0.BUILD-SNAPSHOT spring-projects/spring-amqp@acb5683

@wilkinsona wilkinsona removed the status: blocked An issue that's blocked on an external project change label Aug 19, 2019
@snicoll
Copy link
Member

snicoll commented Aug 19, 2019

@htztomic do you have time to update your proposal. Based on AMQP snapshots, the idea is to introduce a new confirm-type property with the enum type and deprecate publisher-confirms in favour of it while still reading the value (translating a boolean value to the relevant enum value behind the scenes)?

We don't rely on snapshots for AMQP yet but the plan is to do that soon as part of #17898

@garyrussell

This comment has been minimized.

@snicoll

This comment has been minimized.

@snicoll snicoll changed the title Added new property simplePublisherConfirms to RabbitProperties Add confirm-type property to RabbitProperties Aug 20, 2019
@snicoll
Copy link
Member

snicoll commented Aug 20, 2019

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)

@snicoll snicoll added the status: blocked An issue that's blocked on an external project change label Aug 20, 2019
@snicoll snicoll added type: enhancement A general enhancement and removed status: blocked An issue that's blocked on an external project change status: waiting-for-triage An issue we've not yet triaged labels Aug 23, 2019
@snicoll snicoll added this to the 2.2.x milestone Aug 23, 2019
@mbhave mbhave modified the milestones: 2.2.x, 2.2.0.M6 Aug 24, 2019
mbhave pushed a commit that referenced this pull request Aug 24, 2019
mbhave added a commit that referenced this pull request Aug 24, 2019
@mbhave mbhave closed this in 86a9037 Aug 24, 2019
pull bot pushed a commit to scope-demo/spring-boot that referenced this pull request Aug 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants