Skip to content

Contrary to the documentation, setting spring.jms.listener.concurrency alone configures the maximum concurrency #37180

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

Closed

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Sep 1, 2023

Update JMS listener concurrency configuration to set the same minimum and maximum number of consumers when users specify only the minimum using spring.jms.listener.concurrency property.

Prior to this commit, when using spring.jms.listener.concurrency to set the minimum number of consumers without also specifying spring.jms.listener.max-concurrency would result in effective concurrency where the actual minimum number of consumers is always 1, while the maximum number of consumers is the value of spring.jms.listener.concurrency.


This fix results in a breaking change, however the current behavior is simply not aligned with the description of spring.jms.listener.concurrency property.

Also see the javadoc of DefaultMessageListenerContainer#setConcurrency which states:

Specify concurrency limits via a "lower-upper" String, e.g. "5-10", or a simple upper limit String, e.g. "10" (the lower limit will be 1 in this case).

Update JMS listener concurrency configuration to set the same minimum
and maximum number of consumers when users specify only the minimum
using `spring.jms.listener.concurrency` property.

Prior to this commit, when using `spring.jms.listener.concurrency` to
set the minimum number of consumers without also specifying
`spring.jms.listener.max-concurrency` would result in effective
concurrency where the actual minimum number of consumers is always 1,
while the maximum number of consumers is the value of
`spring.jms.listener.concurrency`.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 1, 2023
@wilkinsona
Copy link
Member

Thanks, @vpavic.

I'm tempted to rework the properties a little bit as well. Given the setConcurrency method that takes a String, the spring.jms.listener.concurrency perhaps sets the expectation that it should take a String with the same syntax. To make it clearer that it does not, I wonder if we should rename things. The ideal would be:

  • spring.jms.listener.concurrency.min
  • spring.jms.listener.concurrency.max

This is tricky due to the clash with the existing spring.jms.listener.concurrency property. We may have to settle for something like this instead:

  • spring.jms.listener.min-concurrency
  • spring.jms.listener.max-concurrency

I also wonder if spring.jms.listener.min-concurrency should default to 1. This would make make the "simple upper limit" case explicit such that if you only set max-concurrency we always configure a "lower-upper" string that's 1-n.

There's no need for any updates to the changes proposed here. I'd like to discuss the above with the team so we can decide if and when to change things. It may be that the changes are made across different releases.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Sep 4, 2023
@wilkinsona wilkinsona changed the title Fix handling of JMS listener concurrency properties Contrary to the documentation, setting spring.jms.listener.concurrency alone configures the maximum concurrency Sep 4, 2023
@vpavic
Copy link
Contributor Author

vpavic commented Sep 4, 2023

Your spring.jms.listener.min-concurrency proposal (plus the deprecation of spring.jms.listener.concurrency I assume) seems like the least impactful to developers (because it doesn't change the max property) while also making things more explicit and clear.

If the team decides that's the way forward, I can put together a separate PR for that change as well.

@philwebb philwebb added type: bug A general bug status: noteworthy A noteworthy issue to call out in the release notes and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Sep 18, 2023
@philwebb philwebb added this to the 2.7.x milestone Sep 18, 2023
@wilkinsona wilkinsona self-assigned this Sep 22, 2023
@wilkinsona wilkinsona modified the milestones: 2.7.x, 2.7.17 Sep 22, 2023
wilkinsona pushed a commit that referenced this pull request Sep 22, 2023
Update JMS listener concurrency configuration to set the same minimum
and maximum number of consumers when users specify only the minimum
using `spring.jms.listener.concurrency` property.

Prior to this commit, when using `spring.jms.listener.concurrency` to
set the minimum number of consumers without also specifying
`spring.jms.listener.max-concurrency` would result in effective
concurrency where the actual minimum number of consumers is always 1,
while the maximum number of consumers is the value of
`spring.jms.listener.concurrency`.

See gh-37180
@wilkinsona
Copy link
Member

Thanks very much, @vpavic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: noteworthy A noteworthy issue to call out in the release notes type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants