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

feat: native support for retry template factory #2456

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

klopfdreh
Copy link
Contributor

Fixes: #2439

@klopfdreh
Copy link
Contributor Author

klopfdreh commented Sep 2, 2024

I just saw that the previously mentioned org.springframework.cloud.config.client.ConfigClientHints is not present in the source code. Maybe it should be removed form aot.factories?

Edit I saw that this class is defined in org.springframework.cloud.config.client.ConfigClientAutoConfiguration - I would suggest to move it to top level and merge.

@klopfdreh
Copy link
Contributor Author

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @klopfdreh, looks good - but have added some minor comments. Also, please submit your changes against 4.1.x instead of main, so that we can get them into the 2024.0.x release train as well.

@klopfdreh
Copy link
Contributor Author

I force pushed the change so that they are done against 4.1.x

@artembilan
Copy link

Hi, @OlgaMaciaszek !

Would you mind explaining me why do we need to change that RetryTemplate.logger property at all?
If there is a good reason it might be much better to have access to the property via setter exposed on the RetryTemplate instead of reflection.
Thanks

@artembilan
Copy link

I see ConfigServicePropertySourceLocator propagates its own static Log without any external modification: https://github.com/spring-cloud/spring-cloud-config/blob/main/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServicePropertySourceLocator.java#L125
Same happens with a ConfigServerConfigDataLocationResolver: https://github.com/spring-cloud/spring-cloud-config/blob/main/spring-cloud-config-client/src/main/java/org/springframework/cloud/config/client/ConfigServerConfigDataLocationResolver.java#L69.
So, why do we need to propagate those non-configurable loggers?
Why regular one in RetryTemplate (and anywhere else) is not enough?

@klopfdreh klopfdreh force-pushed the feature/natrtf branch 2 times, most recently from 7fa2921 to de5ba80 Compare September 16, 2024 13:40
@OlgaMaciaszek
Copy link
Contributor

Hi @artembilan, when it comes to Config, I mainly take care of any AOT/ native integrationn issues; @ryanjbaxter or @spencergibb may have more details regarding that implementation, so let's check with them on that.

@klopfdreh klopfdreh changed the base branch from main to 4.1.x September 16, 2024 14:15
@klopfdreh
Copy link
Contributor Author

@OlgaMaciaszek / @artembilan - the build should work, now. I postpone further work until it is clarified if the changes are needed or the retry integration should be refactored.

@ryanjbaxter
Copy link
Contributor

IMO the change should be made here spring-projects/spring-retry#470 if the issue lies in Spring Retry.

@klopfdreh
Copy link
Contributor Author

@ryanjbaxter there are two classes require reflections: RetryTemplateFactory which is located in Spring Cloud Config and RetryTemplate which is located in Spring Retry.

I could modify this PR to only cover the class located in Spring Cloud Config.

@spencergibb
Copy link
Member

spencergibb commented Sep 16, 2024

The first one is in bootstrap before boot configures loggers, the second is for config data and its special deferred logger. I think both are attempts at sane logging in an early phase of cloud or boot respectively

@ryanjbaxter
Copy link
Contributor

@klopfdreh agree, I think that only the classes in Spring Cloud Config should be configured here and configure the RetryTemplate in Spring Retry

@artembilan
Copy link

Thanks, @ryanjbaxter and @spencergibb , for explanation!
So, the setter I suggest in Spring Retry would be OK?

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @klopfdreh. Looks good. Just a cosmetic change to add.

Copy link
Contributor

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit b14ef3e into spring-cloud:4.1.x Sep 16, 2024
@klopfdreh
Copy link
Contributor Author

klopfdreh commented Sep 16, 2024

Thanks a lot for all the help @OlgaMaciaszek / @ryanjbaxter / @spencergibb / @artembilan - we ported 11 of our task applications to Spring Boot Native and I made a lot of changes to various frameworks like AWS SDK V2 for Java / AWS CRT for Java / Spring Cloud AWS / joda-time / github-api / spot bugs / Snappy Java / Micrometer Prometheus RSocket Client to fix issues regarding native images. When the new version of Spring Cloud Data Flow is released we are starting with tests. It is so great that you helped us out here! 👍

@klopfdreh klopfdreh deleted the feature/natrtf branch September 16, 2024 16:04
@OlgaMaciaszek
Copy link
Contributor

Thanks @klopfdreh. Sounds great! Don't hesitate to reach out if there's any other issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native support for RetryTemplateFactory as it uses reflections to access the field logger of RetryTemplate
6 participants