spring-cloud / spring-cloud-commons

Common classes used in different Spring Cloud implementations
Apache License 2.0
704 stars 700 forks source link

refactor: BeanPostProcessor should avoid dependencies on other beans #1304

Closed DanielLiu1123 closed 8 months ago

DanielLiu1123 commented 9 months ago

This PR primarily addresses two issues:

spencergibb commented 9 months ago

Not sure we can make these breaking changes in minor releases.

DanielLiu1123 commented 9 months ago

Including these changes in version 4.1.0 (before the GA) seems reasonable.

spencergibb commented 9 months ago

It's a minor release. We won't make breaking changes

DanielLiu1123 commented 9 months ago

These changes will be accepted?

spencergibb commented 9 months ago

please be patient

app2smile commented 9 months ago

After upgrade to Spring Cloud 2023.0.0 , when I start up my application

[ WARN ]  PostProcessorRegistrationDelegate$BeanPostProcessorChecker.postProcessAfterInitialization(429):  Bean 'org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration$LoadBalancerInterceptorConfig' of type [org.springframework.cloud.client.loadbalancer.LoadBalancerAutoConfiguration$LoadBalancerInterceptorConfig] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). The currently created BeanPostProcessor [lbRestClientPostProcessor] is declared through a non-static factory method on that class; consider declaring it as static instead.

[ WARN ]  PostProcessorRegistrationDelegate$BeanPostProcessorChecker.postProcessAfterInitialization(437):  Bean 'org.springframework.cloud.loadbalancer.config.BlockingLoadBalancerClientAutoConfiguration' of type [org.springframework.cloud.loadbalancer.config.BlockingLoadBalancerClientAutoConfiguration] is not eligible for getting processed by all BeanPostProcessors (for example: not eligible for auto-proxying). Is this bean getting eagerly injected into a currently created BeanPostProcessor [lbRestClientPostProcessor]? Check the corresponding BeanPostProcessor declaration and its dependencies.
crmky commented 9 months ago

Unfortunately the issue was not addressed in Spring Cloud 2023 release. Similar to @app2smile, our applications generates many warnings during startup after upgrade to Spring Boot 3.2 with Spring Cloud 2023.

Those warnings are caused by changes introduced by https://github.com/spring-cloud/spring-cloud-commons/pull/1294. The new introduced LoadBalancerRestClientBuilderBeanPostProcessor depends on either RetryLoadBalancerInterceptor or LoadBalancerInterceptor, which causes the dependencies in the whole dependency chain get early initialized.

It will be good if we can address those warnings in the next release.

OlgaMaciaszek commented 8 months ago

Thanks for creating the PR @DanielLiu1123. As indicated in this comment, while the warnings provide information, we have not noticed this is causing any bugs. In any case, we can consider the change proposal for a future major (date or release train number not set yet), however, please create a separate PR with changes for retry behaviour and a comment with your justification for it, and remove those changes from this one, as we might be considering it independently.

DanielLiu1123 commented 8 months ago

I'd be happy to do that.

OlgaMaciaszek commented 8 months ago

Hello @DanielLiu1123 please keep in mind, that I've already created a PR related to changes in the post-processor after determining there was a bug: https://github.com/spring-cloud/spring-cloud-commons/pull/1319, so please just create a separate PR with your retry logic changes, and we will review it.

DanielLiu1123 commented 7 months ago

Already done, https://github.com/spring-cloud/spring-cloud-commons/pull/1321

OlgaMaciaszek commented 7 months ago

Thanks @DanielLiu1123