spring-attic / app-starters-release

Spring Cloud Stream App Starters and its Release Train
Apache License 2.0
8 stars 14 forks source link

ensure consistency across the various modules #25

Open sabbyanandan opened 8 years ago

sabbyanandan commented 8 years ago

From @sabbyanandan on May 6, 2016 17:9

From @joshlong on December 30, 2015 18:51

I was wading through the source w/ Dr. @markpollack (he was helping me get a cool demo working) and i believe there's an opportunity for some conventions to help module authors:

@EnableBinding is itself transitively a @Configuration class and it ultimately factories MessageChannel instances that any component may inject. It seems odd and assymetrical to stick @EnableBinding on individual messaging endpoints (like Spring Integration service activators etc)

That annotation better belongs on a configuraton class (commonly, i'd expect to see it on the @SpringBootApplication class which is also a @Configuration class. Of course, once that happens then something needs to be done to capture messaging endpoints. @IntegrationComponentScan picks these beans up, or the beans could be returned from @Bean provider methods in a @Configuration class. If we use @IntegrationComponentScan then we'd need to add @MessagingEndpoint to all the components we currently stick @EnableBinding on

we could either as a convention turn on @IntegrationComponentScan and just trust that all the beans get turned on in a given package, implications for composability with other libraries that for some reason happen to be in the same namespace be darned.. or we could decide that all of a given modules components are to be explicitly manufactured in the @SpringBootApplication as @Beans. (Maybe we as framework authors take this approach and endorse either for consumers or other developers just trying to pull together their own solution?)

also, what's the best-of-breed approach for pollers in sources? it seems it's auto-configured for us, so why do some components define the poller explicitly in the annotation for the or provide their own PollerMetadata bean?

So, I propose:

a lot of this stuff is already happening correctly, just.. inconsistently..

Copied from original issue: spring-cloud/spring-cloud-stream-modules#148

Copied from original issue: spring-cloud/spring-cloud-stream-app-starters#69

sabbyanandan commented 8 years ago

From @artembilan on December 30, 2015 22:22

Hi @joshlong !

Thank you for the feedback and Happy New Year!

I haven't read the full comment yet, but let me comment on that what I see.

The @IntegrationComponentScan is exactly for the @MessagingGateway till now. See the source code:

@Import(IntegrationComponentScanRegistrar.class)
public @interface IntegrationComponentScan {

I guess we need one more JavaDocs and Documentation round to make that more clearer: https://jira.spring.io/browse/INT-3927

sabbyanandan commented 8 years ago

From @artembilan on December 30, 2015 22:35

Please, take a look to the @EnableBinding a bit closer:

@Configuration
@Import({ChannelBindingServiceConfiguration.class, AggregateBuilderConfiguration.class, BindingBeansRegistrar.class,
        BinderFactoryConfiguration.class})
@EnableIntegration
public @interface EnableBinding {

Even if it is @Configuration it can be used in conjunction with @SpringBootApplication. It doesn't hurt at all to have several @Configuration on the same class. Finally, all of them is just @Component.

The main feature why that everything works for Spring Integration is @EnableIntegration on the @EnableBinding. That annotation @Import(IntegrationRegistrar.class) with the big Spring Integration stuff, like taskScheduler, messageBuilderFactory, SpEL stuff and so on. Of course, this one registers the Messaging Annotation Processors, too, like for @ServiceActivator, @InboundChannelAdapter and so on. Plus it registers IntegrationConfigurationInitializer from spring.factories, to scan something like:

org.springframework.integration.config.IntegrationConfigurationInitializer=\
org.springframework.integration.dsl.config.DslIntegrationConfigurationInitializer

Without tying to the Core module directly from source code. The same is done for Spring Boot AutoConfiguration, BTW.

sabbyanandan commented 8 years ago

From @artembilan on December 30, 2015 22:47

we could either as a convention turn on @IntegrationComponentScan and just trust that all the beans get turned on in a given package

That does not make so much sense: the Spring Integration infrastructure will be here anyway after @EnableIntegration (@EnableBinding). If you mark your @Bean (or some service method) with the @ServiceActivator, so you expect to have an endpoint there. There is just enough to have standard @ComponentScan to distinguish the desired @Compent s with @ServiceActivator etc.

The @MessageEndpoint (not @MessagingEndpoint, BTW) is redundant, to be honest. The standard @Compent (@Service) is enough for Spring Integration to scan @ServiceActivator or others.

Maybe it's time to @Deprecated it? We don't care about the particular @Component subtype there... CC @markfisher

sabbyanandan commented 8 years ago

From @artembilan on December 30, 2015 22:51

No objections regarding Poller! Looks like we really just can auto-create the PollerMetadata.DEFAULT_POLLER bean and be good without @Poller in the target module code.

sabbyanandan commented 8 years ago

From @artembilan on December 31, 2015 2:24

Well, we have poller auto-config there: ChannelBindingAutoConfiguration based on DefaultPollerProperties. So, I think that should be just improved to cover or possible situations with Poller.

sabbyanandan commented 8 years ago

From @joshlong on January 2, 2016 19:36

Yah I agree having @IntegrationComponentScan on by default isn't great.

Don't deprecate @MessageEndpoint annotation, please. It's a documentation stereotype annotation. @Repository does very little besides exception handling, but it's helpful to document a components role (it's UML stereotype).

sabbyanandan commented 8 years ago

From @artembilan on January 4, 2016 16:13

Well, I'd say that having @IntegrationComponentScan on the @EnableBinding level by default won't hurt. Fully similar to the @ComponentScan on the @SpringBootApplication. From here that suggests us to introduce kinda String[] scanBase* options to the @EnableBinding, like we have in the @SpringBootApplication, but here for the @IntegrationComponentScan.

Yeah! No objections for the @MessageEndpoint and its documentation purpose!