-
Notifications
You must be signed in to change notification settings - Fork 38
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
Issue 252: Allow clients to specify message expiration time. #253
base: master
Are you sure you want to change the base?
Conversation
...c/test/java/com/redhat/jenkins/plugins/ci/integration/AmqMessagingPluginIntegrationTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small test comment but otherwise LGTM
Thanks for the merge request!
|
@olivergondza: I can try to take a stab at answering your question. Assume a situation where a sender is expecting that a certain message is received and processed in some predetermined time by a known consumer. If the message is not processed in that time by the known consumer, the sender retries and resends another message. The sender continues to send the message until the message is processed by its known consumer. If in situations like these, the default value for message expiration is higher than what the sender is willing to wait for, the queue gets flooded with multiple duplicate messages and ends up inundating the consumer with additional burden of processing duplicate messages. If the code allows the sender to override the expiration time with its own custom value, then it will allow the messaging provider to drop the messages that have expired and reduce the overall processing burden on the consumer. |
@olivergondza , I changed to minutes in my latest push. |
Only after reading https://stackoverflow.com/questions/31709353/jms-messageproducer-settimetolive-vs-textmessage-setjmsexpiration did I understand why you chose to set the time-to-live and not the JMSExpiration. Great job with the changes. Looking forward to seeing them get merged soon. |
...ources/com/redhat/jenkins/plugins/ci/pipeline/CIMessageSenderStep/help-timeToLiveMillis.html
Outdated
Show resolved
Hide resolved
@@ -82,7 +92,8 @@ public CIMessageSenderStep(final String providerName, | |||
final MESSAGE_TYPE messageType, | |||
final String messageProperties, | |||
final String messageContent, | |||
Boolean failOnError) { | |||
Boolean failOnError, | |||
Integer timeToLiveMinutes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this needs to be added in https://github.com/jenkinsci/jms-messaging-plugin/blob/master/plugin/src/main/resources/com/redhat/jenkins/plugins/ci/pipeline/CIMessageSenderStep/config.jelly as well, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olivergondza , that change is already in the PR, unless I am missing something.
Allow clients to specify a message time-to-live (in milliseconds) when publishing a messsage.