Skip to content

Add Request Timeouts #1431

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 24, 2025
Merged

Add Request Timeouts #1431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a286381
add timeout
RichardLiu2001 Apr 23, 2025
240b9f0
add iceberg exception mapping
RichardLiu2001 Apr 23, 2025
90d675a
dont use quarkus bom, disable timeout
RichardLiu2001 Apr 24, 2025
6dfe291
nits
RichardLiu2001 Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions api/iceberg-service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ dependencies {
implementation("com.fasterxml.jackson.core:jackson-annotations")
implementation("com.fasterxml.jackson.core:jackson-core")
implementation("com.fasterxml.jackson.core:jackson-databind")

compileOnly(libs.microprofile.fault.tolerance.api)
Copy link
Contributor

@dimas-b dimas-b Apr 24, 2025

Choose a reason for hiding this comment

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

nit: could you move this together with the other compile deps too?

Copy link
Contributor Author

@RichardLiu2001 RichardLiu2001 Apr 24, 2025

Choose a reason for hiding this comment

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

This is the only compile dep :))

}

openApiGenerate {
Expand Down
1 change: 1 addition & 0 deletions api/management-service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ dependencies {
compileOnly(libs.jakarta.annotation.api)
compileOnly(libs.jakarta.inject.api)
compileOnly(libs.jakarta.validation.api)
compileOnly(libs.microprofile.fault.tolerance.api)
compileOnly(libs.swagger.annotations)

implementation(libs.jakarta.servlet.api)
Expand Down
2 changes: 2 additions & 0 deletions api/polaris-catalog-service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ dependencies {
implementation("com.fasterxml.jackson.core:jackson-annotations")
implementation("com.fasterxml.jackson.core:jackson-core")
implementation("com.fasterxml.jackson.core:jackson-databind")

compileOnly(libs.microprofile.fault.tolerance.api)
Copy link
Contributor

@dimas-b dimas-b Apr 24, 2025

Choose a reason for hiding this comment

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

nit: could you move this together with the other compile deps too?

Copy link
Contributor Author

@RichardLiu2001 RichardLiu2001 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is also the only compile dep :))

}

openApiGenerate {
Expand Down
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Copy link

@sfc-gh-rliu sfc-gh-rliu Apr 24, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@eric-maynard eric-maynard Apr 24, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@RichardLiu2001 RichardLiu2001 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@eric-maynard eric-maynard Apr 24, 2025

Choose a reason for hiding this comment

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

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

Copy link

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dimas-b dimas-b Apr 24, 2025

Choose a reason for hiding this comment

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

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

mockito-core = { module = "org.mockito:mockito-core", version = "5.17.0" }
mockito-junit-jupiter = { module = "org.mockito:mockito-junit-jupiter", version = "5.17.0" }
opentelemetry-bom = { module = "io.opentelemetry:opentelemetry-bom", version = "1.49.0" }
Expand Down
4 changes: 4 additions & 0 deletions quarkus/defaults/src/main/resources/application.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ quarkus.otel.sdk.disabled=true

quarkus.test.integration-test-profile=it

quarkus.fault-tolerance.global.timeout.enabled=false
# quarkus.fault-tolerance.global.timeout.unit=minutes
# quarkus.fault-tolerance.global.timeout.value=10

polaris.realm-context.type=default
polaris.realm-context.realms=POLARIS
polaris.realm-context.header-name=Polaris-Realm
Expand Down
1 change: 1 addition & 0 deletions quarkus/service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ dependencies {
implementation("io.quarkus:quarkus-opentelemetry")
implementation("io.quarkus:quarkus-security")
implementation("io.quarkus:quarkus-smallrye-context-propagation")
implementation("io.quarkus:quarkus-smallrye-fault-tolerance")

implementation(libs.jakarta.enterprise.cdi.api)
implementation(libs.jakarta.inject.api)
Expand Down
3 changes: 3 additions & 0 deletions server-templates/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ import {{javaxPackage}}.inject.Inject;

import org.apache.polaris.core.context.RealmContext;

import org.eclipse.microprofile.faulttolerance.Timeout;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -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
Copy link
Contributor

@dimas-b dimas-b Apr 23, 2025

Choose a reason for hiding this comment

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

How is the timeout actually defined?

Copy link
Contributor

@dimas-b dimas-b Apr 23, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@eric-maynard eric-maynard Apr 23, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@RichardLiu2001 RichardLiu2001 Apr 23, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@dimas-b dimas-b Apr 23, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dimas-b dimas-b Apr 24, 2025

Choose a reason for hiding this comment

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

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.

Copy link

@sfc-gh-rliu sfc-gh-rliu Apr 24, 2025

Choose a reason for hiding this comment

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

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

public Response {{nickname}}({{#isMultipart}}MultipartFormDataInput input,{{/isMultipart}}{{#allParams}}{{>queryParams}}{{>pathParams}}{{>headerParams}}{{>bodyParams}}{{^isMultipart}}{{>formParams}},{{/isMultipart}}{{#isMultipart}}{{^isFormParam}},{{/isFormParam}}{{/isMultipart}}{{/allParams}}@Context @MeterTag(key="realm_id",expression="realmIdentifier") RealmContext realmContext,@Context SecurityContext securityContext) {
{{! Don't log form or header params in case there are secrets, e.g., OAuth tokens }}
LOGGER.atDebug().setMessage("Invoking {{baseName}} with params")
Expand Down
2 changes: 2 additions & 0 deletions service/common/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ dependencies {
implementation("com.azure:azure-storage-blob")
implementation("com.azure:azure-storage-file-datalake")

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

testImplementation(platform(libs.junit.bom))
testImplementation("org.junit.jupiter:junit-jupiter")
testImplementation(libs.assertj.core)
Expand Down
2 changes: 2 additions & 0 deletions ...ice/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.polaris.core.exceptions.FileIOUnknownHostException;
import org.eclipse.microprofile.faulttolerance.exceptions.TimeoutException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.event.Level;
Expand Down Expand Up @@ -191,6 +192,7 @@ static int mapExceptionToResponseCode(RuntimeException rex) {
case IllegalArgumentException e -> Status.BAD_REQUEST.getStatusCode();
case UnsupportedOperationException e -> Status.NOT_ACCEPTABLE.getStatusCode();
case WebApplicationException e -> e.getResponse().getStatus();
case TimeoutException e -> Status.REQUEST_TIMEOUT.getStatusCode();
default -> Status.INTERNAL_SERVER_ERROR.getStatusCode();
};
}
Expand Down