fix: Provide a way to customize the graceful shutdown delay of Netty EventLoopGroups
#6528
Conversation
…`EventLoopGroup`s Added parameters to enable setting the quiet period and timeout for graceful shutdown of worker groups, both globally and through the `ClientFactoryBuilder`. Make use of graceful shutdown settings on worker groups on the server side. Refs: line#5813
novoj
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds configurable quiet-period and timeout settings for Netty worker-group graceful shutdown in the Armeria client: new ClientFactoryBuilder APIs store values in ClientFactoryOptions/Flags, and HttpClientFactory uses those values when calling shutdownGracefully on the worker group. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Builder as ClientFactoryBuilder
participant Options as ClientFactoryOptions
participant Factory as HttpClientFactory
participant Netty as EventLoopGroup
User->>Builder: call workerGroupGracefulShutdown*(quiet, timeout)
Builder->>Options: store WORKER_GROUP_*_MILLIS
User->>Factory: close()/shutdown client factory
Factory->>Options: read quietPeriodMillis, timeoutMillis
Factory->>Netty: shutdownGracefully(quiet, timeout, MILLISECONDS)
alt shutdown success
Netty-->>Factory: returns Future success
Factory-->>User: complete close future
else shutdown failure
Netty-->>Factory: returns Future failure
Factory->>Factory: log warning/error
Factory-->>User: complete close future
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (2)
307-319: Add@UnstableApiannotation to the new public options.These are newly added public API fields. As per coding guidelines, new public classes and methods should have the
@UnstableApiannotation. This applies to these newClientFactoryOptionconstants as they are part of the public API surface.Apply this diff:
/** * The graceful worker group pool quiet period in milliseconds. */ + @UnstableApi public static final ClientFactoryOption<Long> WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS = ClientFactoryOption.define("WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS", Flags.defaultClientWorkerGroupGracefulShutdownQuietPeriodMillis()); /** * The graceful worker group pool time out in milliseconds. */ + @UnstableApi public static final ClientFactoryOption<Long> WORKER_GROUP_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS = ClientFactoryOption.define("WORKER_GROUP_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS", Flags.defaultClientWorkerGroupGracefulShutdownTimeoutMillis());
678-690: Add@UnstableApiannotation to the new accessor methods.These are newly added public methods. As per coding guidelines, new public methods should have the
@UnstableApiannotation.Apply this diff:
+ @UnstableApi public long workerGroupGracefulShutdownQuietPeriodMillis() { return get(WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS); } + @UnstableApi public long workerGroupGracefulShutdownTimeoutMillis() { return get(WORKER_GROUP_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java(1 hunks)core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java(2 hunks)core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java(2 hunks)core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java(2 hunks)core/src/main/java/com/linecorp/armeria/common/Flags.java(2 hunks)core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java(1 hunks)core/src/main/java/com/linecorp/armeria/server/Server.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/main/java/com/linecorp/armeria/client/HttpClientFactory.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.javacore/src/main/java/com/linecorp/armeria/common/Flags.java
🧬 Code graph analysis (2)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java (1)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (1)
ClientFactoryOptions(59-775)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (1)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
Flags(106-1939)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Agent
- GitHub Check: Summary
🔇 Additional comments (5)
core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java (1)
498-507: LGTM!The shutdown logic correctly applies the configurable quiet period and timeout values. The listener properly logs failures while ensuring the future completes regardless of the shutdown outcome.
core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java (1)
326-334: Method implementations are correct.The override methods correctly return the corresponding constants. Once the constant values are fixed (see previous comment), these will work as expected.
core/src/main/java/com/linecorp/armeria/server/Server.java (2)
693-698: LGTM!The worker group shutdown correctly uses the server's graceful shutdown configuration with proper millisecond conversion.
719-724: LGTM!The boss group shutdown is consistently configured with the same graceful shutdown settings as the worker group.
core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java (1)
590-620: LGTM!The new provider methods follow the established pattern with proper Javadoc. Since
FlagsProvideris already annotated with@UnstableApi, no additional annotation is needed for these methods. As per coding guidelines, the class-level annotation covers these additions.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR adds customizable graceful shutdown settings for Netty EventLoopGroups on both client and server sides. The motivation is to speed up E2E integration tests by avoiding default graceful shutdown timeouts.
Key changes:
- Implemented server-side graceful shutdown settings for worker and boss groups
- Added new client-side configuration flags for worker group graceful shutdown (quiet period and timeout)
- Provided builder methods in
ClientFactoryBuilderto configure these settings
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Server.java | Applies graceful shutdown settings when stopping worker and boss groups |
| FlagsProvider.java | Adds interface methods for new client worker group shutdown configuration |
| Flags.java | Defines static fields and getters for the new configuration flags |
| DefaultFlagsProvider.java | Defines default values for worker group shutdown settings |
| HttpClientFactory.java | Applies graceful shutdown settings when closing client factory |
| ClientFactoryOptions.java | Defines new options and getters for worker group graceful shutdown |
| ClientFactoryBuilder.java | Adds builder methods to configure worker group graceful shutdown |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Outdated
Show resolved
Hide resolved
…`EventLoopGroup`s Fixes according to automatic review complaints. Refs: line#5813
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
1087-1103: Clarify Javadoc wording for the quiet period behavior.The Javadoc contains inconsistent descriptions of what
{@code 0}means:
- Line 1089 states: "0 disables the quiet period"
- Line 1094 states: "0 disables the graceful shutdown"
These statements are contradictory within the same method's documentation. Setting the quiet period to
0removes only the quiet period, not necessarily the entire graceful shutdown (the timeout can still apply). Consider updating line 1094 to match the more precise wording used elsewhere, such as inClientFactoryBuilderline 931: "0 removes the quiet period entirely."* Returns the default client-side graceful worker group shutdown quiet period in milliseconds. - * {@code 0} disables the quiet period. + * The quiet period allows tasks to complete before shutdown proceeds. * * <p>The default value of this flag is * {@value DefaultFlagsProvider#DEFAULT_CLIENT_WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS}. * Specify the {@code -Dcom.linecorp.armeria.defaultClientWorkerGroupGracefulShutdownQuietPeriodMillis=<long>} - * JVM option to override the default value. {@code 0} disables the graceful shutdown. + * JVM option to override the default value. {@code 0} removes the quiet period entirely. * </p>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java(1 hunks)core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java(2 hunks)core/src/main/java/com/linecorp/armeria/common/Flags.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
🧬 Code graph analysis (1)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java (1)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (1)
ClientFactoryOptions(59-775)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
294-301: LGTM! The swapped wiring issue has been corrected.The constants are now correctly wired to their respective provider methods and flag names. The previous issue where timeout was reading from quiet period and vice versa has been resolved.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java (1)
929-975: LGTM! Worker group graceful shutdown configuration methods are well-implemented.All four methods have been properly annotated with
@UnstableApias requested in previous reviews. The implementation follows established patterns:
- Duration overloads correctly delegate to millisecond variants
- Validation ensures non-negative values
- Options are stored in the appropriate
ClientFactoryOptionskeys- Fluent API is maintained by returning
this- Javadoc clearly describes the behavior of each parameter
…`EventLoopGroup`s Corrected wording. Refs: line#5813
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internal.- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
🧬 Code graph analysis (1)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (1)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
Flags(106-1941)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
Outdated
Show resolved
Hide resolved
…`EventLoopGroup`s Fixes according to automatic review complaints. Refs: line#5813
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the PR.
For 1) consistency across APIs regarding graceful shutdown behavior 2) less methods/flags for setting graceful shutdown fields I'm wondering if the following API makes sense:
- Generalize
GracefulShutdownso that it can be for methods other than server graceful shutdown
As a first step, removeGracefulShutdown#toException. Allow users to specify thetoExceptionbehavior on the server directly
+++ b/core/src/main/java/com/linecorp/armeria/server/GracefulShutdown.java
public interface GracefulShutdown {
...
-
- /**
- * 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);@@ -949,6 +951,14 @@ public final class ServerBuilder implements TlsSetters, ServiceConfigsBuilder<Se
+ @UnstableApi
+ public ServerBuilder gracefulShutdown(
+ GracefulShutdown gracefulShutdown,
+ BiFunction<? super ServiceRequestContext, ? super HttpRequest, ? extends Throwable> toException) {
+ this.gracefulShutdown = requireNonNull(gracefulShutdown, "gracefulShutdown");
+ return this;
+ }
+-
Move
GracefulShutdown,GracefulShutdownBuilder,DefaultGracefulShutdownto the common package (com.linecorp.armeria.common.GracefulShutdown) -
Allow users to specify
GracefulShutdownfor worker groups separately for each server/clientFactory.
e.g.
ClientFactory.builder().workerGroup(workerGroup, GracefulShutdown)
Server.builder().workerGroup(workerGroup, GracefulShutdown)
Flags.workerGroupGracefulShutdown()This will incur some breaking changes, so pinging other @line/dx members in case there is a different opinion
|
Generalizing Currently, when a |
|
I think it's best to start with the direct issue you would like to solve - specifying What do you think of starting with adding the following APIs in ClientFactoryBuilder.java, ClientFactoryOption.java? public ClientFactoryBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnClose,
GracefulShutdown gracefulShutdown) {
}
public ClientFactoryBuilder workerGroup(int numThreads, GracefulShutdown gracefulShutdown) {
} public static final ClientFactoryOption<GracefulShutdown> WORKER_GROUP_GRACEFUL_SHUTDOWN =
ClientFactoryOption.define("WORKER_GROUP_GRACEFUL_SHUTDOWN", Flags.workerGroupGracefulShutdown()); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
1083-1118: Clarify Javadoc for the graceful shutdown settings.The
@UnstableApiannotations are correctly present. However, the Javadoc has some wording that could be clearer:
Line 1090 states
"0 disables the graceful shutdown"but this is misleading—setting the quiet period to 0 only removes the quiet period, not the entire graceful shutdown (the timeout still applies). Consider removing this sentence as line 1085 already clearly states"0 disables the quiet period".Line 1108 states
"0 disables the graceful shutdown"which is redundant with line 1103 that already says"0 disables the timeout and closes the worker group immediately". Consider removing line 1108 to avoid duplication.🔎 Proposed Javadoc refinements
/** * Returns the default client-side graceful worker group shutdown quiet period in milliseconds. * {@code 0} disables the quiet period. * * <p>The default value of this flag is * {@value DefaultFlagsProvider#DEFAULT_CLIENT_WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS}. * Specify the {@code -Dcom.linecorp.armeria.defaultClientWorkerGroupGracefulShutdownQuietPeriodMillis=<long>} - * JVM option to override the default value. {@code 0} disables the graceful shutdown. + * JVM option to override the default value. * </p> * * @see ClientFactoryBuilder#workerGroupGracefulShutdownQuietPeriod(Duration) * @see ClientFactoryBuilder#workerGroupGracefulShutdownQuietPeriodMillis(long) */ @UnstableApi public static long defaultClientWorkerGroupGracefulShutdownQuietPeriodMillis() { return DEFAULT_CLIENT_WORKER_GROUP_GRACEFUL_SHUTDOWN_QUIET_PERIOD_MILLIS; } /** * Returns the default client-side graceful worker group shutdown timeout in milliseconds. * {@code 0} disables the timeout and closes the worker group immediately. * * <p>The default value of this flag is * {@value DefaultFlagsProvider#DEFAULT_CLIENT_WORKER_GROUP_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS}. * Specify the {@code -Dcom.linecorp.armeria.defaultClientWorkerGroupGracefulShutdownTimeoutMillis=<long>} - * JVM option to override the default value. {@code 0} disables the graceful shutdown. + * JVM option to override the default value. * </p> * * @see ClientFactoryBuilder#workerGroupGracefulShutdownTimeout(Duration) * @see ClientFactoryBuilder#workerGroupGracefulShutdownTimeoutMillis(long) */ @UnstableApi public static long defaultClientWorkerGroupGracefulShutdownTimeoutMillis() { return DEFAULT_CLIENT_WORKER_GROUP_GRACEFUL_SHUTDOWN_TIMEOUT_MILLIS; }Based on past review comments, this refinement aligns with feedback from Copilot about removing redundant/contradictory statements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.javacore/src/main/java/com/linecorp/armeria/client/HttpClientFactory.javacore/src/main/java/com/linecorp/armeria/common/Flags.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/common/Flags.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (4)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
294-301: LGTM! Constants are correctly wired.The initialization of the two graceful shutdown constants is now correct, with each reading from the appropriate provider method and flag name. The fix in commit 1fbb70f successfully resolved the swapped wiring issue.
core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java (2)
30-30: LGTM! Import is necessary.The
TimeUnitimport is required for the graceful shutdown configuration on line 469.
466-475: LGTM! Graceful shutdown now uses configurable timing.The worker group shutdown sequence correctly applies the new configurable quiet period and timeout values from
ClientFactoryOptions. The parameters are passed in the correct order toshutdownGracefully(), and the future is properly completed via the listener regardless of success or failure.core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java (1)
935-982: LGTM! New builder methods are correctly implemented.All four graceful shutdown configuration methods are properly implemented:
@UnstableApiannotations are present on all methods as required by coding guidelines- Duration overloads correctly delegate to millisecond overloads
- Validation ensures non-negative values
- Correct
ClientFactoryOptionskeys are used- Javadoc clearly describes the behavior, including the effect of
0- Methods follow the existing builder patterns and return
thisfor fluent chainingThis successfully addresses the past review comments requesting the
@UnstableApiannotations.
…`EventLoopGroup`s Reverting changes in Server implementation. Refs: line#5813
|
Here is what I did:
The client-side implementation remained the same. I think the actual handling is much more aligned with existing methods, such as:
The newly proposed methods by @jrhee17 in the comment at #6528 (comment) suit the server-side API better, in my opinion. The current pull request is minimalistic and still solves my problem. It seems that the delays in my tests are caused by the client rather than the server shutdown. This patch is consistent and can be merged separately from the server-side solution, which I may attempt in a separate pull request. |
Thanks for the update. My thought is that we will end up using the What I had in mind was just accepting GracefulShutdown as-is for now, and treating this PR as the first step that establishes the client-side event loop shutdown behavior in a way that’s consistent with what we’ll want long-term across the codebase. ref: https://github.com/jrhee17/armeria/tree/poc/client-eventloop-graceful Concretely, I’d suggest: Merge this PR on its own merits, but align the public-facing surface area with GracefulShutdown (naming + intent), rather than introducing a parallel concept we’ll later need to reconcile. Then follow up with a second PR that: Once this PR was merged, I was thinking of adding another PR which:
Either I can work on the final clean up, or you can continue with the work if interested. This way we keep the nice property of this changeset being mergeable independently, while also converging on the terminology and abstraction that users (and our own code) are moving towards. Let me know what you think |
4 participants
Added parameters to enable setting the quiet period and timeout for graceful shutdown of worker groups, both globally and through the
ClientFactoryBuilder.Make use of graceful shutdown settings on worker groups on the server side.
Refs: #5813
Motivation:
My E2E integration tests are running slowly just because they wait for default graceful shutdown timeout to expire.
Modifications:
Result:
EventLoopGroups #5813.