Add Request Timeouts #1431

RichardLiu2001 · 2025-04-23T18:42:40Z

Adds @Timeout to request endpoints, configured with a default of 60 seconds.
- https://quarkus.io/guides/smallrye-fault-tolerance
Updates the exception mapping to return 408 for request timeouts.

eric-maynard · 2025-04-23T18:56:36Z

api/iceberg-service/build.gradle.kts

@@ -45,6 +45,9 @@ dependencies {
  implementation("com.fasterxml.jackson.core:jackson-annotations")
  implementation("com.fasterxml.jackson.core:jackson-core")
  implementation("com.fasterxml.jackson.core:jackson-databind")
+
+  compileOnly(platform(libs.quarkus.bom))
+  compileOnly("io.quarkus:quarkus-smallrye-fault-tolerance")


The one doubt I have about this PR is whether we can safely add these dependencies here -- or what the alternative would be if that's not recommended.

cc @snazy / @dimas-b / @singhpk234

Yeah... Why do we need this dep? @RichardLiu2001 ?

To clarify: Even if we go with server-side timeouts, I believe this control should be applied to implementations, not to API definitions.

Can you add more detail here @dimas-b? Within dropwizard, we used a filter for this because that seemed like the best narrow waist for a top-level timeout like this. How can we cover all of the APIs if we push this into implementations?

Why do we need this dep?

This dep is so that IcebergRestCatalogApi can compile.

believe this control should be applied to implementations, not to API definitions

Why? Is it to avoid adding a Quarkus dependency in the API? If so, what is wrong with that?
We also have @Timed at the API level. Why is that acceptable but @Timeout isn't?

@Timed is non-intrusive, hence less of a concern, though.

Compile-only deps should not leak to POM, I guess.

In the end, since API is generated and implementations are meant to be used together with the generated API, I guess it does not make a lot of different from the maintenance POV where the annotation is places.

Sorry about the false alarm.

I'd prefer to not import Quarkus but rather only the dependency that defines the annotation: org.eclipse.microprofile.fault-tolerance:microprofile-fault-tolerance-api

Agreed, cf. #1431 (comment)

Replaced the dependency to use just microprofile-fault-tolerance-api rather than bringing in the Quarkus.

dimas-b · 2025-04-23T19:17:03Z

server-templates/api.mustache

@@ -108,6 +110,7 @@ public class {{classname}}  {
  @Produces({ {{#produces}}"{{{mediaType}}}"{{^-last}}, {{/-last}}{{/produces}} }){{/hasProduces}}{{#hasAuthMethods}}
  {{#authMethods}}{{#isOAuth}}@RolesAllowed("**"){{/isOAuth}}{{/authMethods}}{{/hasAuthMethods}}
  @Timed("{{metricsPrefix}}.{{baseName}}.{{nickname}}")
+  @Timeout


How is the timeout actually defined?

I'm not sure this change is beneficial... Clients should control their own timeouts (in general).

If this annotation cancels the related HTTP request, will it actually cancel the server-side processing?

Within Open Catalog, server-side timeouts have been very useful for protecting the service from very long-running requests. Clients can of course have their own timeouts, but I think server side timeouts are very beneficial.

Agreed that this only makes sense if the server-side processing actually gets cancelled

How is the timeout actually defined?

Quarkus configs - I added a default value of 60s to the default application.properties.

will it actually cancel the server-side processing?

The API method will throw a TimeoutException which gets appropriately mapped in the IcebergExceptionMapper.

The API method will throw a TimeoutException

That is clear :) What happens to the server threads that are still executing the request that timed out?

After reading the docs :) I guess the server thread will get a simple java interrupt, which does look useful. Sorry, I should have paid more attention to this PR initially 🤦

It LGTM (assuming longer default timeput), but let's collect some more reviews, though.

The default is 1 second. To disable the Timeout, we can disable Quarkus fault tolerance which I did in application.properties.

dimas-b · 2025-04-23T23:52:22Z

quarkus/defaults/src/main/resources/application.properties

@@ -86,6 +86,9 @@ quarkus.otel.sdk.disabled=true

 quarkus.test.integration-test-profile=it

+quarkus.fault-tolerance.global.timeout.unit=seconds
+quarkus.fault-tolerance.global.timeout.value=60


IMHO, 60 sec default timeout is too low. I believe we should set the default to allow most calls. Administrators concerned with overload, will have to lower this config based on their specific requirements.

Agreed, maybe the default can be something like 600s or even disabled (is that possible?)

+1 to no timeout by default (if possible), otherwise 600 sec looks reasonable.

Disabled it via application.properties.

dimas-b · 2025-04-24T00:10:37Z

api/iceberg-service/build.gradle.kts

@@ -45,6 +45,9 @@ dependencies {
  implementation("com.fasterxml.jackson.core:jackson-annotations")
  implementation("com.fasterxml.jackson.core:jackson-core")
  implementation("com.fasterxml.jackson.core:jackson-databind")
+
+  compileOnly(platform(libs.quarkus.bom))


Maybe we do not need the Quarkus BOM here. Could we add a specific local version for org.eclipse.microprofile.fault-tolerance:microprofile-fault-tolerance-api to the TOML file? Since the dep is compile-only, version mismatches should not be a problem. WDYT?

I guess this was my main concern initially, but I did not property research and articulate it 🤦

Added a version for microprofile-fault-tolerance-api to the toml file, which is the same version brought in by the current version of Quarkus.

sfc-gh-rliu · 2025-04-24T15:51:48Z

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" }


I used the microprofile-fault-tolerance-api version that is brought in by the current version of Quarkus.

Should we instead rely on the Quarkus BOM for this to prevent conflict? i.e. we would import it as:

dependencies { implementation(enforcedPlatform(libs.quarkus.bom)) implementation("org.eclipse.microprofile.fault-tolerance:microprofile-fault-tolerance-api") }

We never use microprofile-fault-tolerance-api directly as an implementation dependency. We just bring it in as a compile time dependency so that the files in the api / service modules can import the timeout classes and avoid bringing in all of the quarkus dependencies. The runtime dependency will be brought in the quarkus/service module via
implementation("io.quarkus:quarkus-smallrye-fault-tolerance")
which does use the Quarkus BOM.

Yes sorry the implementation part was not really my focus, I suppose it would look like:

compileOnly(enforcedPlatform(libs.quarkus.bom)) compileOnly("org.eclipse.microprofile.fault-tolerance:microprofile-fault-tolerance-api")

How does bringing in the Quarkus BOM prevent conflict? As none of the places where I added the compile time dependency bring in Quarkus, what would it conflict with?

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).

dimas-b · 2025-04-24T17:17:10Z

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)


nit: maybe move up for this to be together with other annotation deps?

dimas-b · 2025-04-24T17:22:31Z

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" }


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).

dimas-b · 2025-04-24T17:25:59Z

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


This LGTM, but I'd still prefer to keep actual timeout properties as an example too (perhaps commented out)

dimas-b · 2025-04-24T17:29:39Z

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)


Should this be an implementation dep because the exception mapper references these classes?

dimas-b · 2025-04-24T17:30:03Z

service/common/build.gradle.kts

@@ -98,6 +100,8 @@ dependencies {

  testImplementation(libs.logback.classic)

+  testImplementation(libs.microprofile.fault.tolerance.api)


Is this needed?

This is needed for the IcebergExceptionMapperTest

eric-maynard

LGTM, thanks for this @RichardLiu2001

add timeout

a286381

github-project-automation bot added this to Basic Kanban Board Apr 23, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Apr 23, 2025

RichardLiu2001 changed the title ~~add timeout~~ Add Request Timeouts Apr 23, 2025

add iceberg exception mapping

240b9f0

RichardLiu2001 marked this pull request as ready for review April 23, 2025 18:51

RichardLiu2001 requested review from adutra, ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, MonkeyCanCode, flyrain and ebyhr as code owners April 23, 2025 18:52

eric-maynard reviewed Apr 23, 2025

View reviewed changes

dimas-b reviewed Apr 23, 2025

View reviewed changes

dimas-b reviewed Apr 24, 2025

View reviewed changes

dont use quarkus bom, disable timeout

90d675a

sfc-gh-rliu reviewed Apr 24, 2025

View reviewed changes

RichardLiu2001 requested review from eric-maynard and dimas-b and removed request for eric-maynard April 24, 2025 16:17

RichardLiu2001 requested a review from sfc-gh-rliu April 24, 2025 16:17

dimas-b reviewed Apr 24, 2025

View reviewed changes

eric-maynard approved these changes Apr 24, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Apr 24, 2025

		@@ -86,6 +86,8 @@ quarkus.otel.sdk.disabled=true

		quarkus.test.integration-test-profile=it

		quarkus.fault-tolerance.enabled=false

		@@ -98,6 +100,8 @@ dependencies {

		testImplementation(libs.logback.classic)

		testImplementation(libs.microprofile.fault.tolerance.api)

Add Request Timeouts #1431

Are you sure you want to change the base?

Add Request Timeouts #1431

Conversation

RichardLiu2001 commented Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardLiu2001 Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-rliu Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-maynard left a comment

Choose a reason for hiding this comment

RichardLiu2001 commented Apr 23, 2025 •

edited

Loading

dimas-b Apr 24, 2025 •

edited

Loading

RichardLiu2001 Apr 24, 2025 •

edited

Loading

sfc-gh-rliu Apr 24, 2025 •

edited

Loading