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

producer.send now returns the JMS Message ID #363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adamretter
Copy link
Contributor

@adamretter adamretter commented Jan 12, 2023

The JMS Message ID is only available after the JMS provider has sent the message. I could not find any mechanism to access the JMS Message ID, and so I made two small changes to the API:

  1. Allow the user to disable JMS Message IDs created by the provider. This is part of the JMS spec - see https://docs.oracle.com/javaee/7/api/javax/jms/JMSProducer.html#setDisableMessageID-boolean-

  2. Modify the producer.send functions to return an F[Option[String]] instead of an F[Unit]. The String is the JMS Message ID. It needs to be an option due to (1) whereby providers can be configured to omit the JMS Message ID. I did consider briefly creating a type alias for the MessageID instead of using String, however it appeared that Option[String] was already used for Message IDs in the JmsMessage trait for the JmsMessage.getJMSMessageId function.

… This commit allows the JmsContext#send methods to return the JMS Message ID if it was set by the underlying implementation
@adamretter adamretter force-pushed the feature/access-to-jms-message-id branch from 45f52cd to cada77f Compare January 13, 2023 09:10
@faustin0
Copy link
Contributor

hi @adamretter thanks for the feature, this can be useful!
i will take a look


def sendNWithDelay(
messageFactory: MessageFactory[F] => F[NonEmptyList[(JmsMessage, (DestinationName, Option[FiniteDuration]))]]
): F[Unit]
): F[NonEmptyList[Option[String]]]
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can simplify this(and the others) return type with a simple
List[String]
returnig a NonEmptyList with Options inside(that can be None) does not make a lot of sense to me :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@faustin0 Do you want me to do that to be able to get this PR merged, or should this be done in a future PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamretter can also be in a future PR but, being a change in a public api, maybe is better to "fix" now the return type of these methods. What do you think?


def send(messageFactory: MessageFactory[F] => F[(JmsMessage, DestinationName)]): F[Unit]
def send(messageFactory: MessageFactory[F] => F[(JmsMessage, DestinationName)]): F[Option[String]]

}

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe adding some "general" docs to the JmsProducer to tell what is returned from these methods can be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I can add something...

_ <- logger.info(s"Pushed ${messages.size} messages.")
_ <- logger.info(s"Consumer to Producer started.\nCollecting messages from output queue...")
received <- Ref.of[IO, Set[String]](Set())
receivedMessages <- receiveUntil(consumer, received, nMessages).timeout(timeout) >> received.get
} yield assert(receivedMessages == bodies)
} yield assert(messages.size == messageIds.size && receivedMessages == bodies)
Copy link
Contributor

@faustin0 faustin0 Jan 25, 2023

Choose a reason for hiding this comment

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

right now the messageIds can be a List of Nones so i would also check that inside the list there is something, like:
messageIds.forAll(_.nonEmpty) .
On the other hand, if the sendN will return a flattened List without the Option this check is not needed anymore.

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