Spring AMQP: Support Both Container Types #9055

garyrussell · 2017-05-01T20:34:10Z

Resolves: #9041

Spring AMQP 2.0 introduced a new container type.

Add support for auto configuration - select container type and separate discrete properties.

Resolves: spring-projects#9041 Spring AMQP 2.0 introduced a new container type. Add support for auto configuration - select container type and separate discrete properties.

snicoll

Thanks for the PR! I've some questions, see individual comments.

snicoll · 2017-05-02T08:44:21Z

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java

+	public enum ContainerType {
+
+		/**
+		 * SimpleMessageListenerContainer.


Can we expand that doc a little? That is showing up in IDEs and being able to understand the benefit would help.

snicoll · 2017-05-02T09:20:46Z

...in/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java

 	@ConditionalOnMissingBean
-	public SimpleRabbitListenerContainerFactoryConfigurer rabbitListenerContainerFactoryConfigurer() {
-		SimpleRabbitListenerContainerFactoryConfigurer configurer = new SimpleRabbitListenerContainerFactoryConfigurer();
+	public RabbitListenerContainerFactoryConfigurer rabbitListenerContainerFactoryConfigurer() {


I think I'd prefer two configurer implementations with dedicated types and a base class. I understand that's quite a lot of code but that probably translates well with what that thing is configuring.

snicoll · 2017-05-02T09:21:37Z

...in/java/org/springframework/boot/autoconfigure/amqp/RabbitAnnotationDrivenConfiguration.java

 	@ConditionalOnMissingBean(name = "rabbitListenerContainerFactory")
-	public SimpleRabbitListenerContainerFactory rabbitListenerContainerFactory(
-			SimpleRabbitListenerContainerFactoryConfigurer configurer,
+	public AbstractRabbitListenerContainerFactory<?> rabbitListenerContainerFactory(


II think I'd prefer two definitions with a @ConditionalOnProperty that translates that if/else below. We prefer to expose the concrete type.

I started down that route, but it didn't work as I expected.

I had @ConditionalOnProperty( ... havingValue = "simple")

and @ConditionalOnProperty( ... havingValue = "direct")

...it didn't work without explicitly setting the property - but I now see there's a matchIfMissing attribute ... doh.

Will rework.

snicoll · 2017-05-02T09:22:01Z

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java


 	public static class Listener {

+		/**


Documentation keys do not start with "The", "A", etc.

snicoll · 2017-05-02T09:24:32Z

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java

 		private final ListenerRetry retry = new ListenerRetry();

+		@NestedConfigurationProperty
+		private final SimpleContainer simple = new SimpleContainer();


So Direct only supports one property? I am a bit confused. The hierarchy of properties are becoming quite complex now so I am wondering if we should create those two namespaces at all.

@snicoll The remaining properties (in Listener) are common to both containers (on the abstract container). The SMLC has 3 properties that are not available with the DMLC and it has one property that's not available on the SMLC. I don't mind having them at the top Listener level and simply ignoring those that don't apply, based on the containerType; it would certainly be better for backwards compatibility. I thought you preferred this hierarchical approach.

OK I get it now. The problem is that there are other nested namespaces that are common. So you have retry that is, and simple and direct, at the same level, that are specific.

I indeed prefer the hierarchical approach but the hierarchy is kind of wrong if you ask me. I have no idea how to fix it though so please do not act on it until we figure it out.

- Remove listener type hierarcy in properties - Add configurer class hierarchy and conditional beans

Move `spring.rabbitmq.listener.*` to `spring.rabbitmq.listener.simple.*` in preparation for Spring AMQP 2.0 that supports different container types. Closes gh-9108 See gh-9055

Add support for auto configuration - select container type and separate discrete properties. See gh-9055

See gh-9055, gh-9108

* pr/9055: Remove deprecated spring.rabbitmq.listener.* properties Polish "Support direct AMQP container" Support direct AMQP container

snicoll · 2017-05-05T10:05:29Z

Thanks @garyrussell - I decided to go a different route with dedicated namespace for the two container types (and a early deprecation of the existing keys in 1.5). I've also simplified the configurer auto-config as we want both of them to be present regardless of the container type (you are free to create as many containers as you want after all).

shakuzen · 2017-05-11T02:25:50Z

...utoconfigure/src/main/java/org/springframework/boot/autoconfigure/amqp/RabbitProperties.java


+		/**
+		 * Container type.
+		 */


I don't know all of the intimate details, but the description above for ContainerType.SIMPLE is that it is the legacy container type. Wouldn't it be better to default to the non-legacy type with the new major version of Spring Boot (and Spring AMQP)?

Unfortunately, it's not black and white - each container has certain pros and cons. I feel that changing the container type without the user explicitly choosing to do so could cause unexpected problems for users.

I prefer that the user explicitly opt-in to using the new container. Documentation here.

Thanks @garyrussell - We've created a dedicated issue to figure this out. Can you please move your comment over there? #9173

spring-projectsGH-9041: Spring AMQP: Support Both Container Types

4f61960

Resolves: spring-projects#9041 Spring AMQP 2.0 introduced a new container type. Add support for auto configuration - select container type and separate discrete properties.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 1, 2017

snicoll changed the title ~~GH-9041: Spring AMQP: Support Both Container Types~~ Spring AMQP: Support Both Container Types May 2, 2017

snicoll mentioned this pull request May 2, 2017

Spring AMQP AutoConfiguration - new Container Type #9041

Closed

snicoll self-assigned this May 2, 2017

snicoll added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 2, 2017

snicoll added this to the 2.0.0.M1 milestone May 2, 2017

snicoll reviewed May 2, 2017

View reviewed changes

garyrussell added 3 commits May 2, 2017 13:26

Polishing - PR Comments

ae82c36

- Remove listener type hierarcy in properties - Add configurer class hierarchy and conditional beans

Fix property name in doc

fbcc855

checkstyle fixes

907c8e6

snicoll mentioned this pull request May 5, 2017

Introduce spring.rabbitmq.listener.simple #9108

Closed

snicoll pushed a commit that referenced this pull request May 5, 2017

Support direct AMQP container

6eddf1b

Add support for auto configuration - select container type and separate discrete properties. See gh-9055

snicoll closed this in 2894e57 May 5, 2017

snicoll added a commit that referenced this pull request May 5, 2017

Remove deprecated spring.rabbitmq.listener.* properties

1e9c1a6

See gh-9055, gh-9108

snicoll added a commit that referenced this pull request May 5, 2017

Merge pull request #9055 from garyrussell:GH-9041

2245bce

* pr/9055: Remove deprecated spring.rabbitmq.listener.* properties Polish "Support direct AMQP container" Support direct AMQP container

shakuzen mentioned this pull request May 11, 2017

Adding Additional Configuration Properties to RabbitAutoConfiguration #5031

Closed

shakuzen reviewed May 11, 2017

View reviewed changes

wilkinsona mentioned this pull request Nov 10, 2023

Minimize the number of beans created by RabbitAnnotationDrivenConfiguration #38067

Closed

Spring AMQP: Support Both Container Types #9055

Spring AMQP: Support Both Container Types #9055

Uh oh!

Conversation

garyrussell commented May 1, 2017

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented May 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

garyrussell May 2, 2017 •

edited

Loading

snicoll May 2, 2017 •

edited

Loading