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

Provide a configuration property for JMS listener container's receive timeout #17332

Closed
wants to merge 8 commits into from

Conversation

ibmmqmet
Copy link
Contributor

The JMS receiveTimeout property spring.jms.template.receive-timeout is not being propagated from a configuration property into the created JMS Listener. That forces the application code to explicitly set it - which is something we find that applications rarely do and the default value is not good for performance in some environments.

This small change copies the external config into the listener.

(I'm told there is now a corporate CLA in place to allow this to be accepted.)

@pivotal-issuemaster
Copy link

@ibmmqmet Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 27, 2019
@wilkinsona
Copy link
Member

Thanks for the proposal.

Given that we have separate properties for the template and the listener, users may be surprised that a spring.jms.template.* property has affected the listener's configuration. I think we should consider a couple of alternatives:

  1. Introduce a separate spring.jms.listener.receive-timeout property
  2. Deprecate spring.jms.template.receive-timeout and add spring.jms.receive-timeout that configures the receive timeout on both the template and the listener

1 provides more flexibility whereas 2 makes it easier to keep the two in sync. I don't have a strong opinion about which is the better option, although I am leaning towards 1. What do you think, @ibmmqmet. @snicoll will probably have an opinion too once he's back in the office.

@wilkinsona wilkinsona added type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 27, 2019
@ibmmqmet
Copy link
Contributor Author

The template seemed to be being used for quite a few other properties, but it's not clear what the real distinction is - it does have that receiveTimeout property inside the JMSProperties.Template class, which is not used elsewhere that I could see - so doing it this way made sense. What's the point of having the property in the class otherwise. And it is configurable into the template, just not propagated. Maybe there's a philosophy around what belongs in a "template" and what belongs in the a "real" created object.

I don't know why you'd treat a JMS Listener differently than a Destination/Connection for default properties, but if there's a preferred way of doing it, then I don't really care.

Of your two options (1) is probably easiest to understand.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 27, 2019
@wilkinsona
Copy link
Member

wilkinsona commented Jun 27, 2019

The template seemed to be being used for quite a few other properties, but it's not clear what the real distinction is - it does have that receiveTimeout property inside the JMSProperties.Template class, which is not used elsewhere that I could see - so doing it this way made sense. What's the point of having the property in the class otherwise.

spring.jms.template properties are used to configure the JmsTemplate bean that's created by JmsAutoConfiguration:

private void mapTemplateProperties(Template properties, JmsTemplate template) {
PropertyMapper map = PropertyMapper.get();
map.from(properties::getDefaultDestination).whenNonNull().to(template::setDefaultDestinationName);
map.from(properties::getDeliveryDelay).whenNonNull().as(Duration::toMillis).to(template::setDeliveryDelay);
map.from(properties::determineQosEnabled).to(template::setExplicitQosEnabled);
map.from(properties::getDeliveryMode).whenNonNull().as(DeliveryMode::getValue)
.to(template::setDeliveryMode);
map.from(properties::getPriority).whenNonNull().to(template::setPriority);
map.from(properties::getTimeToLive).whenNonNull().as(Duration::toMillis).to(template::setTimeToLive);
map.from(properties::getReceiveTimeout).whenNonNull().as(Duration::toMillis)
.to(template::setReceiveTimeout);
}

And it is configurable into the template, just not propagated. Maybe there's a philosophy around what belongs in a "template" and what belongs in the a "real" created object.

It sounds like you weren't aware of Spring Framework's JmsTemplate which simplifies code that's using JMS. That's what the template in the property name is referring to. The JmsTemplate that's created by the auto-configuration and configured using the spring.jms.template.* properties is a real object and is exposed to applications as a bean. On the other hand, the spring.jms.listener properties are using to configure the message listener container that's created by the JMS auto-configuration.

In short:

  • the receive timeout on the auto-configured JmsTemplate controls the timeout for receiving messages when application code is using JmsTemplate to work with JMS
  • the receive timeout on the auto-configured message listener container controls the timeout for the container receiving messages it then passes to any beans that are listening for JMS messages.

In light of the above, is your preference still for the first of the two options that I proposed?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 27, 2019
@wilkinsona
Copy link
Member

wilkinsona commented Jun 27, 2019

(I'm told there is now a corporate CLA in place to allow this to be accepted.)

To associate your GitHub account with IBM's corporate CLA it needs to be a public member of the IBM or OpenLiberty GitHub organisations. There is a little more about this in the CLA tool's FAQ.

@ibmmqmet
Copy link
Contributor Author

Yes, a separate property as in (1) would work.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 28, 2019
@wilkinsona
Copy link
Member

Would you like to update your proposal to that effect?

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 28, 2019
Merge remote-tracking branch 'upstream/master'
@wilkinsona wilkinsona changed the title Make JMS receiveTimeout effective from Boot configuration Provide a configuration property for JMS listener container's receive timeout Jul 2, 2019
@wilkinsona
Copy link
Member

@ibmmqmet Thanks for the updates. Can you please make the necessary changes to have your GitHub account associated with IBM's CLA?

Copy link
Member

@snicoll snicoll left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Would you mind updating JmsAutoConfigurationTests#testJmsListenerContainerFactoryWithCustomSettings to validate the mapping of the receive timeout?

/**
* Timeout to use for receive calls.
*/
private Duration receiveTimeout;
Copy link
Member

Choose a reason for hiding this comment

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

The current default is 1 second so it would be nice to have that documented and tested.

@@ -154,6 +154,11 @@ public void setSessionCacheSize(int sessionCacheSize) {
*/
private Integer maxConcurrency;

/**
* Timeout to use for receive calls.
Copy link
Member

Choose a reason for hiding this comment

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

0 and -1 have special meaning so it would be nice to have those documented here. See the javadoc of org.springframework.jms.listener.AbstractPollingMessageListenerContainer#setReceiveTimeout

@snicoll
Copy link
Member

snicoll commented Aug 17, 2019

@ibmmqmet could we please get some feedback on this?

@ibmmqmet
Copy link
Contributor Author

Sorry for the delay - other projects and being away from my desk got in the way...

I've added the requested comments. But can't add the functions to the test file because the org.springframework.jms.listener.AbstractPollingMessageListenerContainer#getReceiveTimeout() method is marked "protected" (for no obvious reason) so can't be accessed directly.

@spring-projects-issues spring-projects-issues added the status: feedback-provided Feedback has been provided label Aug 23, 2019
@spring-projects-issues spring-projects-issues removed the status: waiting-for-feedback We need additional information before we can continue label Aug 23, 2019
@wilkinsona
Copy link
Member

wilkinsona commented Aug 27, 2019

Thanks for the updates.

method is marked "protected" (for no obvious reason) so can't be accessed directly.

It is marked protected to limit the surface area of the public API.

When we want to test the value of something that isn't publicly accessible we typically use reflection. AssertJ's hasFieldOrPropertyWithValue provides a way of doing that. There are some examples of its usage in several tests in JmsAutoConfigurationTests. Here's one:

assertThat(container).hasFieldOrPropertyWithValue("transactionManager",
context.getBean(JtaTransactionManager.class));

Could you please use this approach to test the new receive timeout property?

In addition to the missing tests, unfortunately we also can't merge this due to the failing CLA check. Our current understanding is that your membership of the IBM or OpenLiberty GitHub organisations needs to be made public.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 27, 2019
@wilkinsona
Copy link
Member

Thanks for the updates. Any news on the CLA?

Unfortunately, if this drags on too much longer we'll have to close the PR as the overhead of monitoring it is starting to outweigh the benefit of the changes.

@ibmmqmet
Copy link
Contributor Author

ibmmqmet commented Sep 5, 2019

CLA seems to be signed now

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 5, 2019
@pivotal-issuemaster
Copy link

@ibmmqmet Thank you for signing the Contributor License Agreement!

@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Sep 5, 2019
@wilkinsona wilkinsona self-assigned this Sep 5, 2019
@wilkinsona wilkinsona closed this in 795a044 Sep 5, 2019
@wilkinsona
Copy link
Member

Thanks very much for making your first contribution to Spring Boot, @ibmmqmet. I'm pleased that we managed to get there in the end.

@wilkinsona wilkinsona added this to the 2.2.0.M6 milestone Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants