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

Publisher confirm support #115

Closed
feelyou opened this issue Aug 23, 2019 · 11 comments · Fixed by #116
Closed

Publisher confirm support #115

feelyou opened this issue Aug 23, 2019 · 11 comments · Fixed by #116
Assignees
Milestone

Comments

@feelyou
Copy link

feelyou commented Aug 23, 2019

We are planning migrating from ActiveMQ to RabbitMQ. We need the at-least-once delivery guarantee and the response time is critical to our application. So we want to use publisher confirm extension but it is not supported in JMS client.
I want to know why this feature is not an option in JMS client. Do you consider the JMS specification compliance or other policy? If I fork it and add the feature to RMQConnection#createChannel, will there be some unknown problems?
Thanks in advance!

@michaelklishin michaelklishin changed the title why not support publisher confirm? Publisher confirm support Aug 23, 2019
@acogoluegnes
Copy link
Contributor

Publisher confirms would be an interesting feature to add, but it is asynchronous by nature and the JMS API does not provide a callback after publishing.

One possible integration without extending the JMS API would be to add a flag to enable publisher confirms and block until confirm arrives for each outbound message. This would be transparent for JMS applications but would perform terribly (don't expect more than a few 100's of messages per second, even less, depending on the scenario). Nevertheless it could work for applications that favor API compliance and reliability other throughput.

Another integration would be to implement the publisher confirms asynchronous API inside the JMS client, based on the AMQP client one. Performance would be better than with the first option, but this would be super leaky and terrible in terms of API compliance. I'd rather suggest using directly the AMQP client instead of trying to push its API inside inside the JMS client.

@lukebakken
Copy link

I'd rather suggest using directly the AMQP client instead of trying to push its API inside inside the JMS client

👍 to this.

@feelyou
Copy link
Author

feelyou commented Aug 26, 2019

@acogoluegnes Thank you for your reply.
IMHO, using publisher confirms and a default callback with empty methods has same effect to automatic confirms. Maybe there could be some extension point for application developers to build thire own implementions of ConfirmListener while not interfering JMS spec.
If there's any misunderstand please point it to me.
Thanks a lot!

@michaelklishin
Copy link
Member

An API extension is what we will investigate. Unfortunately retrofitting a client to do what it was not meant to do can end up with sub-optimal results. Hence the suggestion to use the client that already has publisher confirm support.

@feelyou
Copy link
Author

feelyou commented Aug 26, 2019

@michaelklishin Thanks for the explanation.
Just because RabbitMQ has better response time, we consider to migrate from ActiveMQ to RabbitMQ. Response time along with at-lease-once guarantee are more concerned than throughput. And if we use AMQP publisher with JMS consumer, it would be much dangerous than modifying rabbit-jms-client.

In the perspective of standard and public library, maybe it's not a good option to support publisher confirms. I think 'fire and forgot' has little difference to 'recieve confirm and ignore', so I want to fork it and use it privately, is there any bad influence?

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Aug 26, 2019

RMQConnectionFactory could have a ConfirmListener property like the following one:

interface ConfirmListener {

  void handleAck(Message message);

  void handleNack(Message message);

}

This is different from the AMQP Java client (no sequence number, no multiple flag), but the JMS client would handle the difference, making the developer's like easier in this case. This is basically what Reactor RabbitMQ does.

@michaelklishin @feelyou WDYT?

@michaelklishin
Copy link
Member

@acogoluegnes if it works for Reactor RabbitMQ, I'd say let's do it.

@feelyou
Copy link
Author

feelyou commented Aug 26, 2019

@acogoluegnes Thanks! I think rabbitmq-jms-client shoud do sufficent but as less as possible work, how about just add addConfirmListener method to RMQConnectionFactory(using existing ConfirmListener interface inside rabbitmq-java-client) and leave other responsibilities to application developer?

@acogoluegnes
Copy link
Contributor

@feelyou The AMQP client ConfirmListener methods refer to the publishing sequence number (aka delivery tag), which has no JMS equivalent. Without this there's no way to correlate the publisher confirm to the original message.

What I'm suggesting with this new interface is to abstract this away from the user of the JMS client.

@acogoluegnes
Copy link
Contributor

I forgot to mention in my proposal that if the ConfirmListener is not set, the underlying AMQP channels will never be set to use publisher confirms, making this feature an opt-in, without any impact if it's not turned on.

@feelyou
Copy link
Author

feelyou commented Aug 26, 2019

@acogoluegnes Got it, thanks!

acogoluegnes added a commit that referenced this issue Aug 28, 2019
Add an optional callback to the RMQConnectionFactory to be notified
of confirmed/nack-ed published messages. Setting this callback will
enable publisher confirms globally (for all the children objects created
by the RMQConnectionFactory instance).

Publisher confirms use a publishing sequence number to correlate
messages. As there is no such concept in JMS, the JMS client takes care
of mapping sequence numbers with original messages.

The confirm callback has one method with one context parameter. This
allows adding more information about published messages later (e.g. the
session or producer instance), without breaking changes. The context
consists now of the original message and a flag for confirmed or nacked.

Fixes #115
acogoluegnes added a commit to rabbitmq/rabbitmq-website that referenced this issue Aug 28, 2019
@acogoluegnes acogoluegnes added this to the 1.13.0 milestone Aug 28, 2019
@acogoluegnes acogoluegnes self-assigned this Aug 28, 2019
acogoluegnes added a commit that referenced this issue Aug 29, 2019
Add an optional callback to the RMQConnectionFactory to be notified
of confirmed/nack-ed published messages. Setting this callback will
enable publisher confirms globally (for all the children objects created
by the RMQConnectionFactory instance).

Publisher confirms use a publishing sequence number to correlate
messages. As there is no such concept in JMS, the JMS client takes care
of mapping sequence numbers with original messages.

The confirm callback has one method with one context parameter. This
allows adding more information about published messages later (e.g. the
session or producer instance), without breaking changes. The context
consists now of the original message and a flag for confirmed or nacked.

Fixes #115

(cherry picked from commit 48382b2)

Conflicts:
	pom.xml
	src/main/java/com/rabbitmq/jms/admin/RMQConnectionFactory.java
	src/main/java/com/rabbitmq/jms/client/RMQMessageProducer.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants