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

Allow building @LoadBalanced RestClient in component constructor. #1339

Merged

Conversation

OlgaMaciaszek
Copy link
Collaborator

Load balanced RestClient with deferring postprocessor.

@OlgaMaciaszek OlgaMaciaszek added this to the 4.1.2 milestone Feb 19, 2024
@OlgaMaciaszek OlgaMaciaszek self-assigned this Feb 19, 2024
@Bean
@ConditionalOnMissingBean
public LoadBalancerRequestFactory loadBalancerRequestFactory(LoadBalancerClient loadBalancerClient) {
return new LoadBalancerRequestFactory(loadBalancerClient, transformers);
}

@Configuration(proxyBeanMethods = false)
@AutoConfiguration
static class DeferringLoadBalancerInterceptorConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these inner classes get autoconfigured because the outerclass is specified in spring.factories?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; the new annotation is just same as @Configuration(proxyBeanMethods = false)

@OlgaMaciaszek OlgaMaciaszek merged commit 5b5bf58 into main Feb 20, 2024
3 checks passed
*/
@Deprecated(forRemoval = true)
public class LoadBalancerRestClientBuilderBeanPostProcessor implements BeanPostProcessor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going nuts why this was deprecated and the bean removed when I initially saw this one. I was going in my head with : "is there now a new way to post-process beans? How can a @LoadBalanced be captured without a BeanPostProcessor?"

I was planning to open this defect today, but then saw you actually reverted and fixed :)

@OlgaMaciaszek
Copy link
Collaborator Author

I'm really sorry for the mess around that deprecation @wind57. In fact, it was being load-balanced with the previous change, however, with the caveat that a component constructor would be to early to build the RestClient instance with a @LoadBalanced RestClient.Builder, which is why it's been reworked again.

@wind57
Copy link
Contributor

wind57 commented Feb 21, 2024

My point was rather different, I'm sorry for not being verbose enough.

Before this PR, I saw that LoadBalancerRestClientBuilderBeanPostProcessor that was a @Bean, was removed to be such and deprecated. Now, the question I had, was how is it possible then to find out who is actually annotated with @LoadBalanced, if not a BeanPostProcessor exists?

Yes, you did add SmartInitializingSingleton and a customizer, but doesn't that mean that load-balancing will be applied to every instance of RestClinet.Builder, be that annotated or not with @LoadBalanced, simply because its done for everyone. There is no logical check of "if bean is annotated with @LoadBalanced" : do that...

I hope I make more sense now. And don't worry about the issue at all!

@OlgaMaciaszek
Copy link
Collaborator Author

The @LoadBalanced annotation over in:
@LoadBalanced @Autowired(required = false) private List<RestClient.Builder> restClientBuilders = Collections.emptyList();
works as a qualifier; that's also how it's always been done for RestTemplate, just too early for RestClient for some of the scenarios, since there we work with builder instances and not actual client instances.

@wind57
Copy link
Contributor

wind57 commented Feb 21, 2024

oh my, I'm an idiot, you're right! I completely missed the @Qualifier in @LoadBalanced. Thank you for taking the time to explain.

@OlgaMaciaszek
Copy link
Collaborator Author

No worries, it's not very straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants