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

add support for multiple ContainerCustomizer (spring-amqp) #32617

Closed
rfelgent opened this issue Oct 6, 2022 · 8 comments
Closed

add support for multiple ContainerCustomizer (spring-amqp) #32617

rfelgent opened this issue Oct 6, 2022 · 8 comments
Labels
for: external-project For an external project and not something we can fix status: declined A suggestion or change that we don't feel we should currently apply

Comments

@rfelgent
Copy link

rfelgent commented Oct 6, 2022

Hello community,

I have a feature request.

Background: I have splitted up my ContainerCustomizer logic into multiple parts in order to increase focus (one takes care of timeouts, another one takes care of consumer concurency and so on).

Currently, SpringBoots auto-configuration for ContainerCustomizer allows only one instance at a time.

@Bean(name = "rabbitListenerContainerFactory")
	@ConditionalOnMissingBean(name = "rabbitListenerContainerFactory")
	@ConditionalOnProperty(prefix = "spring.rabbitmq.listener", name = "type", havingValue = "simple",
			matchIfMissing = true)
	SimpleRabbitListenerContainerFactory simpleRabbitListenerContainerFactory(
			SimpleRabbitListenerContainerFactoryConfigurer configurer, ConnectionFactory connectionFactory,
			ObjectProvider<ContainerCustomizer<SimpleMessageListenerContainer>> simpleContainerCustomizer) {
		SimpleRabbitListenerContainerFactory factory = new SimpleRabbitListenerContainerFactory();
		configurer.configure(factory, connectionFactory);
		simpleContainerCustomizer.ifUnique(factory::setContainerCustomizer);   // here is the "problem"
		return factory;
	}

My currenct work around is to provide one ContainerCustomizier as an adapter which delegates the work to each ContainerCustomizier, like this:

 ...
     // SpringBoot defaults
     // simpleContainerCustomizer.ifUnique(factory::setContainerCustomizer);
      List<ContainerCustomizer<SimpleMessageListenerContainer>> containerCustomizers = simpleContainerCustomizer.orderedStream().collect(Collectors.toList());
      if (!containerCustomizers.isEmpty()) {
        factory.setContainerCustomizer(container -> containerCustomizers.forEach(customizer -> customizer.configure(container))); // here comes the wrapper/delegate logic
      }

I know that I could encapsulate all me customization into one ContainerCustomizer but,

  • I would like to keep things simple/small and consistent by splitting along its purpose/usage (catchword: seperation of concerns)
  • there are other customizers in SpringBoot auto-configure which do allow multiple implementations at the same time, e.g. WebClientCustomizer

Please let me know if my feature request to add logic that can handle multiple ContainerCustomizer` is feasable/useful.

If so, I could provide a pull request.

@rfelgent rfelgent changed the title add support for _multiple_ ContainerCustomizer add support for multiple ContainerCustomizer (spring-amqp) Oct 6, 2022
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 6, 2022
@wilkinsona
Copy link
Member

wilkinsona commented Oct 6, 2022

We've taken our lead from Spring AMQP which supports a single customizer. We could make your workaround part of Boot but that feels like we'd be going in a direction that Spring AMQP has chosen not to support. In short, I think you should raise this with with Spring AMQP. If the team decides that AbstractRabbitListenerContainerFactory should support more than one ContainerCustomizer we can then adapt to that in Boot.

I'm going to close this issue for now as there's nothing that we can do or would want to do in Boot at this time. If a change is made in Spring AMQP we can re-open and update Boot accordingly.

/cc @garyrussell

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Oct 6, 2022
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply for: external-project For an external project and not something we can fix and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 6, 2022
@garyrussell
Copy link
Contributor

A simple solution, which retains the separation of concerns would be to implement a CompositeContainerCustmizer which takes a collection of delegate customizers; that pattern is in use elsewhere in the portfolio.

If you would like to implement that, and contribute it to spring-amqp, no Boot changes would be required (as long as you don't declare the delegates as beans).

@rfelgent
Copy link
Author

rfelgent commented Oct 8, 2022

@garyrussell this kind of delegate/adapter code is my current solution and I was thinking of moving such logic too spring-boot auto-config.

It is not clear too me why the composite/adapter code must reside inside spring-amqp?! Why an Adapter class in spring-amqp and not just a list?
And yes, my delegated classes are spring managed beans - that is my current requirement.

@garyrussell and @wilkinsona please make a final decision where to put the new code: either spring-boot or spring-amqp (or do both projects require changes?!)

I can then create a pull request.

Thank you for your time.

@garyrussell
Copy link
Contributor

If the composite customizer resides in spring-amqp, it would be available to users that don't use Spring Boot (as well as Boot users). It makes more sense too, because the interface resides there.

It would be a simple addition there (plus some small doc updates) and would not require any boot changes - the Boot team are extremely busy right now.

@rfelgent
Copy link
Author

rfelgent commented Oct 18, 2022

fyi: the PR for introducing CompositeContainerCustomizer was accepted in spring-amqp and is available since v2.4.8

As soon as spring-boot (2.7.x or 3.0.x) references this new release, I will make a PR for spring-boot.

@garyrussell
Copy link
Contributor

Why do you need changes to Boot? Just add the composite customizer as a single bean, don't declare the delegates as beans, or mark the composite as @Primary, and Boot will auto configure it with existing code.

@rfelgent
Copy link
Author

rfelgent commented Oct 18, 2022

ah... very nice idea @garyrussell.

the thing is, that your solution is a little bit different compared to the configuration of other customizers in spring-boot, e.g. the WebClientCustomizer. The auto-configuration of WebClientCustomizer supports multiple WebClientCustomizer beans at a time. I do not know how "other" customizers behave or should behave - I came accross the ContainerCustomizer which behaves differently. It took me some time to figure out why none of my n (n > 1) ContainerCustomizer beans was found until I checked the auto-config

simpleContainerCustomizer.ifUnique(factory::setContainerCustomizer);   // here is the "problem"

I would love to have "one" way to handle all kinds of customizers and I thought, that spring boot autoconfig is the abstraction to facilate the customizer handling (see my 1st comment of this issue).

@garyrussell
Copy link
Contributor

I understand it is a different technique.

My personal preference is the composite approach; it is simpler and you are in total control of the sequence in which customizers are called. With the Boot approach you have to implement Ordered or add an @Order annotation, to ensure they are invoked in the right sequence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: external-project For an external project and not something we can fix status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants