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

Handle forwarded Direct Reply To and non-Direct Reply To Q's #280

Merged
merged 3 commits into from
Apr 28, 2023
Merged

Handle forwarded Direct Reply To and non-Direct Reply To Q's #280

merged 3 commits into from
Apr 28, 2023

Conversation

alehane
Copy link
Contributor

@alehane alehane commented Apr 27, 2023

Reason for update - we have a system that uses IBMMQ via JMS and are converting some operations to use RabbitMQ, rather than IBMMQ. To be able to maintain backwards compatibility and remain technology agnostic, we're using the Rabbit-JMS-Client. rather than than the native rabbitmq client.

This update is a bug fix for allowing the rabbitmq-jms-client to correctly respond to handle the following use cases:

  • Replying correctly to a message containing a direct reply to queue by ensuring the rabbit correlation id is set (as well as the JMS Correllation id).

  • Forwarding on a message containing a direct reply to queue to a further consumer where a future consumer will reply to the initial client.

  • Handling a reply to queue that's not a "direct reply to" or a forwarded
    "direct reply to" queue.

Change Details

RMQMessageProducer:

  • Replying to a direct reply to - this has been acheived by ensuring that the JMSCorrelationId is set as the rabbit mq correllation id, as well as being set as the jms property form of the correlation id

  • Rename maybeSetReplyToPropertyToDirectReplyTo to setReplyToProperty to allow any reply to queue to be sent onto the consumer, not just direct reply to initialisations. This allows the rabbit-jms-client to handle forwarded direct-reply-to and non-direct reply to destinations.

RMQMessage:

  • Rename maybeSetupDirectReplyTo to setupJMSReplyTo and update to be able to handle non-direct reply to queues. This code assumes that the reply to queue will be hosted on the same exchange as the messaging being sent

    I'm not sure if this is a valid assumption or whether this should potentially allow the use of:

    • 'reply to queue name' - Use the same exchange

    • 'reply to queue name'/'exchange name' - Allow an exchange name
      to be set for the reply to queue.

Reason for update - we have a system that uses IBMMQ via JMS and are
converting some operations to use RabbitMQ, rather than IBMMQ. To be
able to maintain backwards compatibility and remain technology agnostic,
we're using the Rabbit-JMS-Client. rather than than the native
rabbitmq client.

This update is a bug fix for allowing the rabbitmq-jms-client
to correctly respond to handle the following use cases:

 - Replying correctly to a message containing a direct reply to
   queue by ensuring the rabbit correlation id is set (as well as the
   JMS Correllation id).

 - Forwarding on a message containing a direct reply to queue to
   a further consumer where a future consumer will reply to
   the initial client.

 - Handling a reply to queue that's not a "direct reply to" or a
forwarded
   "direct reply to" queue.

Change Details

RMQMessageProducer:

 - Replying to a direct reply to - this has been acheived by ensuring
   that the JMSCorrelationId is set as the rabbit mq correllation id,
   as well as being set as the jms property form of the correlation id

 - Rename maybeSetReplyToPropertyToDirectReplyTo to setReplyToProperty
   to allow any reply to queue to be sent onto the consumer, not just
   direct reply to initialisations. This allows the rabbit-jms-client
   to handle forwarded direct-reply-to and non-direct reply to
   destinations.

RMQMessage:

 - Rename maybeSetupDirectReplyTo to setupJMSReplyTo and update to be
   able to handle non-direct reply to queues. This code assumes that
   the reply to queue will be hosted on the same exchange as the
   messaging being sent

   I'm not sure if this is a valid assumption or whether this should
   potentially allow the use of:

     - <reply to queue name> - Use the same exchange

     - <reply to queue name>/<exchange name> - Allow an exchange name
       to be set for the reply to queue.
@michaelklishin
Copy link
Member

Perhaps it's only a matter of terminology used but

  • There is no such thing as a "direct reply queue", it's a convention-based mechanism that explicitly avoids creation of very short-lived queues
  • I'm not sure what you mean by a "future consumer". Direct reply to is set up by consumers, and it is very common to register a consumer before sending any direct reply-to requests

@michaelklishin michaelklishin changed the title Bug Fix - Handle Forwarded Direct Reply To and Non Direct Reply To Q's Handle forwarded Direct Reply To and non-Direct Reply To Q's Apr 27, 2023
@alehane
Copy link
Contributor Author

alehane commented Apr 27, 2023

Hi @michaelklishin,

Apologies if I've muddled the terminology!

To try and elaborate on the "future consumer", we have the following use case:

  • External client posts a message to rabbit mq with a reply to queue
  • Component (a) picks up the message, does some processing and then needs to forwards the message onto Component (b)
  • Component (b) picks up the message, does some additional processing, then uses the original reply-to queue to message the external client.

Let me know if you want any more details,

@acogoluegnes
Copy link
Contributor

We cannot assume the reply-to destination is on the same exchange as the initial message as soon as direct reply-to is not used, applications can have set up something else. There are Rpc*IT test classes, the ones not using direct reply-to are broken with these changes.

The RMQMessage.maybeSetupDirectReplyTo changes are not required.
@alehane
Copy link
Contributor Author

alehane commented Apr 27, 2023

Hi @acogoluegnes,

ah, ok, it looks like my changes in RMQMessage are not required, I'll roll them back.

I've updated the unit tests I've written for RMQMessage, as I'm rolling back the code change for this class, should I also remove the unit test's I've added?

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Apr 27, 2023

The tests can stay if they check existing features, it does not do any harm :)

@acogoluegnes acogoluegnes modified the milestones: 2.8.0, 2.9.0 Apr 28, 2023
@acogoluegnes acogoluegnes merged commit bbb3300 into rabbitmq:2.x.x-stable Apr 28, 2023
@acogoluegnes
Copy link
Contributor

Thanks!

@alehane
Copy link
Contributor Author

alehane commented Apr 28, 2023

Many thanks @acogoluegnes!

Do you know what the release schedule for 2.x.x is and when 2.9.0 may be released?

Ta!

github-actions bot pushed a commit that referenced this pull request Apr 28, 2023
Handle forwarded Direct Reply To and non-Direct Reply To Q's
@acogoluegnes
Copy link
Contributor

@alehane It took a bit of time but 2.9.0 is out: https://github.com/rabbitmq/rabbitmq-jms-client/releases/tag/v2.9.0.

@alehane
Copy link
Contributor Author

alehane commented Oct 16, 2023

Many thanks @acogoluegnes, much appreciated!

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.

3 participants