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

Enhance AOT-Support (Issue#3171) #3193

Merged
merged 11 commits into from
Mar 19, 2024

Conversation

wisskirchenj
Copy link
Contributor

@wisskirchenj wisskirchenj commented Dec 23, 2023

Changes:

  • replace registerBeanDefinition callback with a factoryMethod
  • split out a RouterFunctionHolderFactory from the Registrar class and provide its bean in the AutoConfiguration
  • set refresh scope on the RouterFunctionHolder bean only if RefreshScope bean definition registered and add warning
  • added first bunch of runtime hints, that supports most handlers, predicates and filters as external route config (see comment below)

Remarks:
fixes #3171

  • I have signed the Contributor License Agreement
  • The major code changes come from a mere extraction of a new RouterFunctionHolderFactory class from the Registrar (similar to what Josh did in his hacking session on UTube recently) in order to make this a bean and thus accessible for factory methods. My first solution was to just create the whole GatewayMvcPropertiesBeanDefinitionRegistrar class as bean in addition to its @Import-role as ImportBeanDefinitionRegistrar. But I did not like this too much, as it puts "too much hats" on this class.
    However, if the amount of code changes scares you off now, this partial solution (not including the Runtime Hints) with just like 6 LOC changes is still available as branch wisskirchenj@a718c27
  • The refresh scope on the RouterFunctionHolder (for cloud config support I guess) is only set, if there is a RefreshScope bean definition in the registry. This is exactly the case iff the property spring.cloud.refresh.enabled is not explicitly set to false. So in order to not crash in AOT, the user has to explicitly set this - but it seems to be in accordance with Spring Cloud Config, where there even is an explicit warning in the docs to do that for AOT (might be useful to add in gateway mvc docs too..). Still I think, the newbie to gateway-mvc will likely not set / know that. That is why I gave an instructive warning in the Registrar class.

replace registerBeanDefinition callback with a factoryMethod
split out a RouterFunctionHolderFactory from the Registrar class and provide its bean in the AutoConfiguration
set refresh scope on the RouterFunctionHolder bean only if RefreshScope bean in context and add warning
update outdated comment
add runtimeHintsRegistrar to allow externalized route configuration
@wisskirchenj
Copy link
Contributor Author

wisskirchenj commented Dec 23, 2023

now added and imported RuntimeHintsRegistrar to support externalized configs as the one I tested with below (properties-style for readability :-)).
RuntimeHints are definitely not complete yet though - e.g. I did not include Reflection Binds on RetryFilterFunctions as this would have been Conditional on dependencies - but it should be a good start for a lot of native image use cases, I hope...

The application.properties settings, that run smoothly as native image in my local test setup:

spring.cloud.refresh.enabled=false

spring.cloud.gateway.mvc.routes[0].id=recipe
spring.cloud.gateway.mvc.routes[0].uri=http://localhost:8080/
spring.cloud.gateway.mvc.routes[0].predicates[0]=Path=/recipe/**
spring.cloud.gateway.mvc.routes[0].filters[0]=TokenRelay=
spring.cloud.gateway.mvc.routes[0].filters[1]=PrefixPath=/api

spring.cloud.gateway.mvc.routes[1].id=code
spring.cloud.gateway.mvc.routes[1].uri=http://localhost:8090/
spring.cloud.gateway.mvc.routes[1].predicates[0]=Path=/code/**
spring.cloud.gateway.mvc.routes[1].filters[0]=TokenRelay=

spring.cloud.gateway.mvc.routes[2].id=quiz
spring.cloud.gateway.mvc.routes[2].uri=http://localhost:9090/
spring.cloud.gateway.mvc.routes[2].predicates[0]=Path=/quiz/**
spring.cloud.gateway.mvc.routes[2].filters[0]=TokenRelay=
spring.cloud.gateway.mvc.routes[2].filters[1]=PrefixPath=/juergen/api

@wisskirchenj
Copy link
Contributor Author

@spencergibb May I politely request your review on this?

marcusdacoregio added a commit to marcusdacoregio/rinha-backend-2024q1 that referenced this pull request Feb 18, 2024
@wisskirchenj

This comment was marked as off-topic.

@ryanjbaxter

This comment was marked as off-topic.

@wisskirchenj
Copy link
Contributor Author

wisskirchenj commented Feb 20, 2024

@ryanjbaxter: Thanks for the reply.

I think you want to check who was the author of that tweet, it doesn’t appear to be @spencergibb 😉

No :-) I authored it myself. But it was in exchange and on request of @spencergibb.

We all have a lot of work on our plates we will try to get to it soon

Sorry - I patiently waited for 2 months. I was just worried, it might get lost, though it can add value.

@ryanjbaxter
Copy link
Contributor

No I mean the tweet about working for Cockroach was not from Spencer, he just retweeted it.

I promise we will get to it eventually, we appreciate the PR!

@spencergibb
Copy link
Member

Not going anywhere

@maradanasai
Copy link

I'm waiting for this PR getting release

@spencergibb
Copy link
Member

@maradanasai Please watch https://github.com/spring-cloud/spring-cloud-release/milestones this will be part of 2023.0.1 in mid march

@spencergibb
Copy link
Member

I want @OlgaMaciaszek to look, and she will be back next week. The release has been rescheduled for later this month, 2024-03-26.

# Conflicts:
#	spring-cloud-gateway-server-mvc/src/main/java/org/springframework/cloud/gateway/server/mvc/config/GatewayMvcPropertiesBeanDefinitionRegistrar.java
resolve merge conflicts
@wisskirchenj
Copy link
Contributor Author

@spencergibb @OlgaMaciaszek
I just resolved the merge conflicts by transferring adapted methods (mainly making operationArgs now Map<String, Object>) inside GatewayMvcPropertiesBeanDefinitionRegistrar::routerFunctionHolderSupplier (and subordinated methods) to the RouterFunctionHolderFactory in my PR-branch.

maven verify passes fine.

@spencergibb
Copy link
Member

Re-running build. Still want @OlgaMaciaszek to look

@wisskirchenj
Copy link
Contributor Author

Still want @OlgaMaciaszek to look

Yes, sure!

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.

Hello @wisskirchenj, the hints implementation looks good, but have added some minor comments. Please address.

@wisskirchenj
Copy link
Contributor Author

Hello @wisskirchenj, the hints implementation looks good, but have added some minor comments. Please address.

@OlgaMaciaszek Thank you for your review. I have incorporated all your comments - except the one with the package modifier on Registrar class, as with the present locations of Registrar and AutoConfiguration classes public is needed.

@spencergibb spencergibb merged commit fbb72e2 into spring-cloud:main Mar 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway Server MVC support for AOT
6 participants