-
Notifications
You must be signed in to change notification settings - Fork 973
fix: Provide a way to customize the graceful shutdown delay of Netty EventLoopGroup`s #6563
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
f4d64ba
02f102d
c62f5e9
56fd159
7232047
628c8d4
ef5cfb2
9398c3d
09bb8b1
ec355b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1292,4 +1292,15 @@ default ResponseTimeoutMode responseTimeoutMode() { | |||||
| default Boolean annotatedServiceContentLogging() { | ||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Provides a configuration for a graceful shutdown of a worker group. | ||||||
| * The method returns an instance of {@link GracefulShutdown}, which specifies | ||||||
| * the shutdown strategy for the worker group. By default, this method | ||||||
| * disables the graceful shutdown process. | ||||||
| */ | ||||||
| @UnstableApi | ||||||
| default GracefulShutdown workerGroupGracefulShutdown() { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to other reviewers: I think it's fine to skip the |
||||||
| return GracefulShutdown.disabled(); | ||||||
|
||||||
| return GracefulShutdown.disabled(); | |
| return null; |
Also, to keep backwards compatibility, what do you think of returning netty's default as the default value?
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 will update it like this, but I wonder if this check would be still valid then:
if (!shutdownOnClose) {
checkArgument(gracefulShutdown == GracefulShutdown.disabled(),
"GracefulShutdown is not supported when shutdownOnClose is false");
}| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,15 +14,12 @@ | |
| * under the License. | ||
| */ | ||
|
|
||
| package com.linecorp.armeria.server; | ||
| package com.linecorp.armeria.common; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although this class is annotated with @UnstableApi, it cannot be moved to a different package. This is to ensure we maintain backward compatibility for as many users as possible. How about adding a new one and deprecating the old one?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original proposal was to move the class and incur a breaking change:
Since this is a balance of maintaining two sets of classes vs. incurring breaking changes, I guess this is somewhat subjective and could go either way. I'm fine with either approach
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After a team discussion, we have decided to introduce a breaking change for the following reasons:
@novoj Sorry for confusing you. 😉
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's okay. I didn't react because I anticipated there might be a discussion about it. Both approaches have their benefits and disadvantages. |
||
|
|
||
| import java.time.Duration; | ||
|
|
||
| import com.linecorp.armeria.common.HttpRequest; | ||
| import com.linecorp.armeria.common.HttpResponse; | ||
| import com.linecorp.armeria.common.HttpStatus; | ||
| import com.linecorp.armeria.common.ShuttingDownException; | ||
| import com.linecorp.armeria.common.annotation.UnstableApi; | ||
| import com.linecorp.armeria.server.Server; | ||
|
|
||
| /** | ||
| * Configures the graceful shutdown behavior of a {@link Server}. | ||
|
|
@@ -57,12 +54,4 @@ static GracefulShutdown disabled() { | |
| */ | ||
| Duration timeout(); | ||
|
|
||
| /** | ||
| * Returns an {@link Throwable} to terminate a pending request when the server is shutting down. | ||
| * The exception will be converted to an {@link HttpResponse} by {@link ServerErrorHandler}. | ||
| * | ||
| * <p>If null is returned, the request will be terminated with {@link ShuttingDownException} that will be | ||
| * converted to an {@link HttpStatus#SERVICE_UNAVAILABLE} response. | ||
| */ | ||
| Throwable toException(ServiceRequestContext ctx, HttpRequest request); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -14,17 +14,12 @@ | |||||
| * under the License. | ||||||
| */ | ||||||
|
|
||||||
| package com.linecorp.armeria.server; | ||||||
| package com.linecorp.armeria.common; | ||||||
|
|
||||||
| import static com.linecorp.armeria.server.DefaultServerConfig.validateNonNegative; | ||||||
| import static java.util.Objects.requireNonNull; | ||||||
|
|
||||||
| import java.time.Duration; | ||||||
| import java.util.function.BiFunction; | ||||||
|
|
||||||
| import com.linecorp.armeria.common.HttpRequest; | ||||||
| import com.linecorp.armeria.common.HttpStatus; | ||||||
| import com.linecorp.armeria.common.ShuttingDownException; | ||||||
| import com.linecorp.armeria.common.annotation.UnstableApi; | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -36,20 +31,25 @@ public final class GracefulShutdownBuilder { | |||||
| // Defaults to no graceful shutdown. | ||||||
| private static final Duration DEFAULT_GRACEFUL_SHUTDOWN_QUIET_PERIOD = Duration.ZERO; | ||||||
| private static final Duration DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT = Duration.ZERO; | ||||||
| private static final BiFunction<ServiceRequestContext, HttpRequest, Throwable> DEFAULT_ERROR_FUNCTION = | ||||||
| (ctx, req) -> ShuttingDownException.get(); | ||||||
|
|
||||||
| static final GracefulShutdown DISABLED = GracefulShutdown.builder().build(); | ||||||
|
|
||||||
| private Duration quietPeriod = DEFAULT_GRACEFUL_SHUTDOWN_QUIET_PERIOD; | ||||||
| private Duration timeout = DEFAULT_GRACEFUL_SHUTDOWN_TIMEOUT; | ||||||
| private BiFunction<ServiceRequestContext, HttpRequest, Throwable> toException = DEFAULT_ERROR_FUNCTION; | ||||||
|
|
||||||
| GracefulShutdownBuilder() {} | ||||||
|
|
||||||
| static Duration validateNonNegative(Duration duration, String fieldName) { | ||||||
|
||||||
| static Duration validateNonNegative(Duration duration, String fieldName) { | |
| private static Duration validateNonNegative(Duration duration, String fieldName) { |
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.
Agree
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.
Optional) What do you think of validating if
shutdownOnClose == truesinceGracefulShutdownwon't have meaning unless theworkerGroupis actually closed?e.g.
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.
Of course, this makes sense.