Skip to content

Conversation

garyrussell
Copy link
Contributor

JIRA: https://jira.spring.io/browse/AMQP-806

Avoid complex infrastructure for simple use cases.

JIRA: https://jira.spring.io/browse/AMQP-806

Avoid complex infrastructure for simple use cases.
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's all.
Looks like cool feature! 😄

com.rabbitmq.client.ConfirmCallback nacks);
----

NOTE: These `ConfirmCallback` objects (for acks and nacks) are the Rabbit client callbacks, not the template callback.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we really can't use there our own org.springframework.amqp.rabbit.core.RabbitTemplate.ConfirmCallback ?
It doesn't look too hard to delegate calls from the internal Rabbit client ConfirmCallback instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't really fit; we don't have correlation data or a cause or a way to convey multiple.

I suppose I could subclass CorrelationData to provide the tag and multiple. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I see what you mean.
No, I think it's really not necessary to overhead here: since we don't have anything to wait for, therefore we don't need correlation.

So, the current solution is fine.

Will pull locally for deeper review and possible merging...

* @param acks a confirm callback for acks.
* @param nacks a confirm callback for nacks.
* @param <T> the return type.
* @return the result of the action method.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@since 2.1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add this method to RabbitOperations - can you do that if you find no more problems in the review?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops - too late.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just add subsequent commit.
Not a big deal though.

Thanks for the pointer! My bad 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed via bf5498d

@artembilan
Copy link
Member

Merged as 3da5931

@artembilan artembilan closed this Mar 28, 2018
@garyrussell garyrussell deleted the AMQP-806 branch March 30, 2018 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants