dimas-b commented on code in PR #1431:
URL: https://github.com/apache/polaris/pull/1431#discussion_r2058907966
##########
api/management-service/build.gradle.kts:
##########
@@ -42,6 +42,8 @@ dependencies {
compileOnly("io.micrometer:micrometer-core")
implementation(libs.slf4j.api)
+
+ compileOnly(libs.microprofile.fault.tolerance.api)
Review Comment:
nit: maybe move up for this to be together with other annotation deps?
##########
service/common/build.gradle.kts:
##########
@@ -90,6 +90,8 @@ dependencies {
implementation("com.azure:azure-storage-blob")
implementation("com.azure:azure-storage-file-datalake")
+ compileOnly(libs.microprofile.fault.tolerance.api)
Review Comment:
Should this be an `implementation` dep because the exception mapper
references these classes?
##########
service/common/build.gradle.kts:
##########
@@ -98,6 +100,8 @@ dependencies {
testImplementation(libs.logback.classic)
+ testImplementation(libs.microprofile.fault.tolerance.api)
Review Comment:
Is this needed?
##########
quarkus/defaults/src/main/resources/application.properties:
##########
@@ -86,6 +86,8 @@ quarkus.otel.sdk.disabled=true
quarkus.test.integration-test-profile=it
+quarkus.fault-tolerance.enabled=false
Review Comment:
This LGTM, but I'd still prefer to keep actual timeout properties as an
example too (perhaps commented out)
##########
gradle/libs.versions.toml:
##########
@@ -73,6 +73,7 @@ javax-servlet-api = { module =
"javax.servlet:javax.servlet-api", version = "4.0
junit-bom = { module = "org.junit:junit-bom", version = "5.12.2" }
logback-classic = { module = "ch.qos.logback:logback-classic", version =
"1.5.18" }
micrometer-bom = { module = "io.micrometer:micrometer-bom", version = "1.14.6"
}
+microprofile-fault-tolerance-api = { module =
"org.eclipse.microprofile.fault-tolerance:microprofile-fault-tolerance-api",
version = "4.1.1" }
Review Comment:
Compile-only deps have very little effect downstream, if I'm not mistaken.
The only tricky situation I can imagine is when annotation packages or class
names change... which is very unlikely for a mature library.
I do not think pulling Quarkus into API modules is justified for that.
During the server build, Quarkus will (should) bump all versions according
to its platform deps anyway.
However, we should probably have a Quarkus integration test that overrides
the timeout to a low value and validates that requests do get cancelled. This
should validate proper annotation handling (in case of class name changes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
