-
Notifications
You must be signed in to change notification settings - Fork 923
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
Add Spring Boot 3 integration modules #4574
Conversation
Motivation: Spring 6 and Spring Boot 3 have been released. https://spring.io/blog/2022/11/24/spring-boot-3-0-goes-ga Note that the added modules will not be published until we refactor `spring-boot2-autoconfigure` internals. Modifications: - Add Spring Boot 3 integrations symmetirically with Spring Boot 2. - `publish` flags are not added intentionally. - Set the release target for Spring Boot 3 and examples to Java 17. - Add `PortUtil` and migrate off `SocketUtils`. - Migrate AutoConfiguration in spring.factories to `o.s.b.a.AutoConfiguration.imports`. https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.7-Release-Notes#new-autoconfiguration-annotation - Change `management.metrics.export.defaults.enabled` to `management.promethus.metrics.export.enabled` in tests spring-projects/spring-boot#30381 - Fix compile errors in the webflux integration - Migrate moved `LocalServerPort` and `LocalManagementPort`. Depedencies: - Add Spring Boot 3.0.0 - Add `hibernate-validator 8.0.0 - Bump Micrometer into 1.10.2 - Add Jakarta dependencies - jakarta-inject 2.0.1 - jakarta-validation 3.0.2 - jakarta-websocket 2.1.0 Result: Experimentally set up Spring Boot 3 integrations.
...nfigure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpRequest.java
Outdated
Show resolved
Hide resolved
zookeeper3/src/test/java/com/linecorp/armeria/common/zookeeper/ZooKeeperTestUtilTest.java
Outdated
Show resolved
Hide resolved
…/ZooKeeperTestUtilTest.java
…armeria/spring/web/reactive/ArmeriaServerHttpRequest.java
This reverts commit a51f97e.
Rescheduled the milestone to 1.23.0 |
When can this PR be merged? This is a very much awaited PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! 👍 👍 👍
Left nits and questions.
core/src/main/java/com/linecorp/armeria/internal/common/util/PortUtil.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/common/util/PortUtil.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/internal/common/util/PortUtilTest.java
Show resolved
Hide resolved
...webflux-security/src/test/java/com/linecorp/armeria/spring/security/SecureConfiguration.java
Show resolved
Hide resolved
...java/com/linecorp/armeria/spring/web/reactive/AbstractServerHttpResponseVersionSpecific.java
Show resolved
Hide resolved
...figure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpResponse.java
Show resolved
Hide resolved
api libs.jakarta.inject | ||
compileOnly libs.jakarta.validation | ||
// We may remove jakarta.websocket dependencies if Armeria webflux module supports WebSocket. | ||
implementation libs.jakarta.websocket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we need this or not. Shouldn't the user who wants to use WebSocket include this dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/spring-projects/spring-framework/blob/main/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/support/HandshakeWebSocketService.java#L272-L291
WebFlux in Spring Boot 3 uses Jakarta Websocket as a fallback if no custom WebSocket RequestUpgradeStrategy
is detected. The explanation was not enough. Let me update the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the explanation. 🙇
zookeeper3/src/test/java/com/linecorp/armeria/common/zookeeper/ZooKeeperTestUtilTest.java
Show resolved
Hide resolved
Sorry about the delay. 😓 |
core/src/test/java/com/linecorp/armeria/internal/common/util/PortUtilTest.java
Show resolved
Hide resolved
spring/boot3-autoconfigure/src/test/resources/config/application-autoConfTest.yml
Outdated
Show resolved
Hide resolved
zookeeper3/src/test/java/com/linecorp/armeria/common/zookeeper/ZooKeeperTestUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left only nits and questions 😄 Thanks for the handling this issue @ikhoon 🙇
core/src/main/java/com/linecorp/armeria/internal/common/util/PortUtil.java
Outdated
Show resolved
Hide resolved
@@ -4,6 +4,11 @@ management: | |||
prometheus: | |||
enabled: true # In order to inject PrometheusMeterRegistry | |||
|
|||
prometheus: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that
- spring-boot 1,2:
management.metrics.export.prometheus.enabled
- spring-boot 3:
prometheus.metrics.export.enabled
Which is why both are added. Am I understanding correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I put both properties to share this configuration file. We can check out the change properties in
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0.0-Configuration-Changelog
- management.metrics.export.prometheus.enabled
+ management.prometheus.metrics.export.enabled
// called. It can lead to | ||
// leaking of the buffer when an | ||
// HttpResponse is canceled. | ||
return writeWithInternal(Mono.just(buffer).doOnSubscribe(s -> subscribed.set(true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have plans to file an issue/fix upstream also? If I understand correctly this also affects upstream. Having it fixed there would simplify code from our side a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly this also affects upstream.
It might be a problem only on our side. If we didn't recover the CancelledSubscriptionException
exception,
https://github.com/line/armeria/blob/master/spring/boot3-webflux-autoconfigure/src/main/java/com/linecorp/armeria/spring/web/reactive/ArmeriaServerHttpResponse.java#L93-L95
the buffer would be released by
Line 187 in 96ee8e8
}).doOnError(ex -> DataBufferUtils.release(buffer)).doOnCancel(() -> { |
Anyway, it would be worth discussing it with upstream maintainers. Let me create an issue for it.
spring/boot3-autoconfigure/src/test/resources/config/application-settings.yml
Outdated
Show resolved
Hide resolved
} | ||
|
||
tasks.compileJava.dependsOn(generateSources) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) Is there no need for compileTestJava
to depend on generateSources
(or compileJava
)?
I realize that this is just a copy of tomcat8
, but this seemed odd 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, compileTestJava
depends compileJava
by the Java Plugin.
https://docs.gradle.org/current/userguide/java_plugin.html
So we don't need an additional dependency for them.
./gradlew :tomcat9:compileTestJava --dry-run
:core:compileJava SKIPPED
:junit4:copyJunitSources SKIPPED
:junit4:generateSources SKIPPED
:junit4:compileJava SKIPPED
:junit5:compileJava SKIPPED
:testing-internal:compileJava SKIPPED
:tomcat9:generateSources SKIPPED
:tomcat9:compileJava SKIPPED
:tomcat9:versionProperties SKIPPED
:tomcat9:processResources SKIPPED
:tomcat9:classes SKIPPED
:tomcat9:compileTestJava SKIPPED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the conflicts are resolved. Thanks for adding this long-awaited module!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, @ikhoon!
Thanks a lot! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice! Thanks @ikhoon ! 👍 🚀 🙇
Thanks for the review. I missed pushing some changes that should be checked in before the final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM 🚀
Motivation: Spring Boot 3 has been released and we added new modules for Spring Boot 3 line#4574. Spring also stopped support for Spring Boot 1 a long time ago. https://spring.io/blog/2018/07/30/spring-boot-1-x-eol-aug-1st-2019 See line#4651 for the details Modifications: - Remove `boot1-autoconfigure` and `boot1-starter` modules Result: - You no longer use Spring 1 integration with the lastet Armeria version. - Closes line#4651
Motivation: Spring Boot 3 has been released and we added new modules for Spring Boot 3 #4574. Spring also stopped support for Spring Boot 1 a long time ago. https://spring.io/blog/2018/07/30/spring-boot-1-x-eol-aug-1st-2019 See #4651 for the details Modifications: - Remove `boot1-autoconfigure` and `boot1-starter` modules Result: - You no longer use Spring 1 integration with the latest Armeria version. - Closes #4651
Motivation:
Spring 6 and Spring Boot 3 have been released.
https://spring.io/blog/2022/11/24/spring-boot-3-0-goes-ga
Modifications:
jakarta.*
) instead of EE 8 (javax.*
).PortUtil
and migrate offSocketUtils
.META-INF/spring.factories
too.s.b.a.AutoConfiguration.imports
. https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-2.7-Release-Notes#new-autoconfiguration-annotationmanagement.metrics.export.defaults.enabled
tomanagement.promethus.metrics.export.enabled
in tests Move metrics export properties spring-projects/spring-boot#30381AbstractServerHttpRequest.getMethod()
became non-default method.LocalServerPort
andLocalManagementPort
.org.springframework.boot.web.server.LocalServerPort
->org.springframework.boot.test.web.server.LocalServerPort
Dependencies:
Result:
You can now use Spring Boot 3 with Armeria.