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 pluggable strategy for dealing with reply to queues. #299

Conversation

alehane
Copy link
Contributor

@alehane alehane commented Jun 2, 2023

Add a pluggable strategy for handling reply-to properties when messages are received with a reply-to specified.

This will default to the standard behaviour of just handling "amq.rabbitmq.reply-to" destinations.

An alternative strategy, ReturnToSenderExchangeReplyToStrategy, has also been implemented that assumes that reply-to queues, if not "amq.rabbitmq.reply-to", are to be returned on the same exchange as the received message.

An alternative reply-to strategy can be set on the connection factory using the setReplyToStrategy method.

For a little background, we have a component that is insisting on using temporary queues (created with no name) as a reply to property. As the reply to queues in rabbit MQ can be rather ambiguous as to what exchange the client expects a response to be send over, I've added pluggable behaviour to the rabbitmq client, which defaults to the existing behaviour.

I've tried to follow keep the flow and setting of the strategy in keeping with how the rest of the jms client is configured, but I'm happy to change or rename classes if needed.

Add a pluggable strategy for handling reply to queues when
messages are received with the a reply to specified.

This will default to the standard behaviour of just handling
"amq.rabbitmq.reply-to" destinations.

An alternative stategy, ReturnToSenderExchange, has also been
implemented that assumes that reply to queues, if not
"amq.rabbitmq.reply-to" are returned on the same exchange as the
received message.

The reply to strategy can be set on the connection factory.
@michaelklishin
Copy link
Member

Thank you for taking the time but there is one thing that I fail to understand.

As long as you know the name of the queue, you can publish "directly" to it via the default exchange. That is, the exchange with the name of "", which is always bound to every queue with a routing key equal to that queue's name.

And consumers do not care what exchange was used to send a response.

Were you not aware of the default exchange and this special feature? I'm not sure I understand
the need for this change, since you can publish a response to a server-named queue just by knowing its name.

@alehane
Copy link
Contributor Author

alehane commented Jun 2, 2023

Hi @michaelklishin,

Thanks for the update, I was not aware that publishing to the default exchange would go directly to the queue in question.

However, I think that currently, if a reply to queue is received by the rabbitjms-client that does not start with "amq.rabbitmq.reply-to", then the client will discard that queue in the RMQMessage's maybeSetupDirectReplyTo method, so I think there's still a need to be able to alter the default behaviour of the client's reply to handling.

If you agree, I'm happy to rename/update the ReturnToSenderExchangeReplyTo so that it uses the default exchange, rather than the source exchange?

@michaelklishin
Copy link
Member

Yeah, let's at least use the default exchange for publishing when only knowing the name of the sender/request-reply publisher's queue.

 - Renamed ReturnToSenderExchangeReplyToStrategy to
HandleAnyReplyToStrategy.
 - Updated HandleAnyReplyToStrategy to use the default exchange,
   rather than the received exchange.
@alehane
Copy link
Contributor Author

alehane commented Jun 2, 2023

Cool, sounds good to me. I've renamed ReturnToSenderExchangeReplyToStrategy to a, probably slightly better (and shorter) name: HandleAnyReplyToStrategy. (again, open to any better suggestions!!!) and updated the exchange to use the default exchange.

@acogoluegnes acogoluegnes added this to the 3.2.0 milestone Jun 5, 2023
@acogoluegnes acogoluegnes merged commit 3c29c0f into rabbitmq:2.x.x-stable Jun 5, 2023
github-actions bot pushed a commit that referenced this pull request Jun 5, 2023
…tegy

Add pluggable strategy for dealing with reply to queues.
acogoluegnes added a commit that referenced this pull request Jun 5, 2023
Depends on JMS API, not RabbitMQ JMS classes. Do not include
receiving JMS destination in parameters.

References #299
@alehane alehane deleted the 2.x.x-Add_Pluggable_ReplyTo_Strategy branch June 5, 2023 08:33
github-actions bot pushed a commit that referenced this pull request Jun 5, 2023
Depends on JMS API, not RabbitMQ JMS classes. Do not include
receiving JMS destination in parameters.

References #299
@acogoluegnes
Copy link
Contributor

Thanks for this contribution.

@alehane I pushed a couple of changes: afd778b.

I removed the receiving destination from the parameters of the RetryToStrategy#handleReplyTo method. It was not used in the 2 implementations. Do you see any cases where it would be necessary?

@alehane
Copy link
Contributor Author

alehane commented Jun 5, 2023

Hi @acogoluegnes,

No problems, thanks for accepting the change

The receiving destination was only used to obtain the exchange used to send the original request (before I knew that just using the default exchange would work). I left that in, just in case anyone else implementing the strategy needed this information to make any decisions about how to handle the reply to (or perhaps for potential debug info), but I don't mind whether it's removed.

acogoluegnes added a commit that referenced this pull request Jun 5, 2023
Depends on JMS API, not RabbitMQ JMS classes. Do not include
receiving JMS destination in parameters.

References #299

(cherry picked from commit afd778b)

Conflicts:
	src/main/java/com/rabbitmq/jms/client/DefaultReplyToStrategy.java
	src/main/java/com/rabbitmq/jms/client/HandleAnyReplyToStrategy.java
	src/main/java/com/rabbitmq/jms/client/ReplyToStrategy.java
	src/test/java/com/rabbitmq/jms/client/RMQSessionTest.java
github-actions bot pushed a commit that referenced this pull request Jun 5, 2023
Depends on JMS API, not RabbitMQ JMS classes. Do not include
receiving JMS destination in parameters.

References #299

(cherry picked from commit afd778b)

Conflicts:
	src/main/java/com/rabbitmq/jms/client/DefaultReplyToStrategy.java
	src/main/java/com/rabbitmq/jms/client/HandleAnyReplyToStrategy.java
	src/main/java/com/rabbitmq/jms/client/ReplyToStrategy.java
	src/test/java/com/rabbitmq/jms/client/RMQSessionTest.java
jjank pushed a commit to jjank/rabbitmq-jms-client that referenced this pull request Sep 12, 2024
Depends on JMS API, not RabbitMQ JMS classes. Do not include
receiving JMS destination in parameters.

References rabbitmq#299
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants