fix: Provide a way to customize the graceful shutdown delay of Netty EventLoopGroup`s #6563
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 GracefulShutdown for EventLoopGroups and related wiring: moves GracefulShutdown to common, removes per-request toException on GracefulShutdown, introduces GracefulShutdownExceptionFactory, exposes flags/options/providers, and applies quietPeriod/timeout when shutting down worker and boss groups for server and client factories. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Operator as Operator (user code)
participant ServerBuilder as ServerBuilder
participant Server as Server
participant Worker as EventLoopGroup (worker)
participant Boss as EventLoopGroup (boss)
participant Handler as HttpServerHandler
note right of ServerBuilder `#DDEBF7`: configure GracefulShutdown & factory
Operator->>ServerBuilder: builder.gracefulShutdown(gs, factory)
Operator->>ServerBuilder: builder.workerGroup(..., gsWorker)
Operator->>ServerBuilder: builder.build()
ServerBuilder->>Server: construct(config with gs, gsWorker, gsBoss, factory)
note right of Server `#E2F0D9`: running...
Operator->>Server: stop()
Server->>Worker: shutdownGracefully(quietMillis, timeoutMillis)
Server->>Boss: shutdownGracefully(quietMillis, timeoutMillis)
alt pending requests during shutdown
Server->>Handler: gracefulShutdownExceptionFactory.createThrowableForContext(ctx, req)
Handler->>Operator: request terminated with produced Throwable (if any)
end
Worker->>Server: terminationComplete
Boss->>Server: terminationComplete
Server->>Operator: stopped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.java⚙️ CodeRabbit configuration file
Files:
⏰ 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)
🔇 Additional comments (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: 3
🧹 Nitpick comments (5)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdown.java (1)
24-28: Javadoc is now incomplete after package relocation.The Javadoc states this "Configures the graceful shutdown behavior of a {@link Server}" but now that
GracefulShutdownis in the common package and used forClientFactoryworker groups as well, this description is incomplete.Consider updating to reflect the broader usage:
/** - * Configures the graceful shutdown behavior of a {@link Server}. + * Configures the graceful shutdown behavior of worker groups for both + * {@link com.linecorp.armeria.server.Server Server} and + * {@link com.linecorp.armeria.client.ClientFactory ClientFactory}. */core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (1)
75-81: Worker-group graceful shutdown option is wired correctly
WORKER_GROUP_GRACEFUL_SHUTDOWNis defined with a default fromFlags.workerGroupGracefulShutdown(), so existing code that does not override it still has a deterministic policy.- The
workerGroupGracefulShutdown()accessor simply returns this option and is marked@UnstableApi, which is appropriate for a newly introduced tuning knob.You might optionally consider documenting in the accessor Javadoc that it defaults to the
Flags.workerGroupGracefulShutdown()value, for discoverability, but functionally this looks good.Also applies to: 756-763
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (3)
566-572: Consider breaking long method signature for readability.The method signature spans a single long line. For consistency with other overloads in this file (e.g., line 537), consider breaking the parameters across multiple lines.
🔎 Proposed formatting
- public ServerBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnStop, GracefulShutdown gracefulShutdown) { + public ServerBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnStop, + GracefulShutdown gracefulShutdown) {
574-585: Javadoc missing description forgracefulShutdownparameter.The Javadoc for
workerGroup(int numThreads, GracefulShutdown gracefulShutdown)doesn't document thegracefulShutdownparameter. Consider adding a@paramtag for consistency with the three-argument overload at lines 557-565.🔎 Proposed Javadoc addition
/** * Uses a newly created {@link EventLoopGroup} with the specified number of threads for * performing socket I/O and running {@link Service#serve(ServiceRequestContext, Request)}. * The worker {@link EventLoopGroup} will be shut down when the {@link Server} stops. * * @param numThreads the number of event loop threads + * @param gracefulShutdown the {@link GracefulShutdown} configuration for the worker {@link EventLoopGroup} */
1007-1010: Parameter nametoExceptionis inconsistent with field name.The parameter is named
toException, but the field it sets is namedgracefulShutdownExceptionFactory. Consider renaming for consistency:🔎 Proposed fix
@UnstableApi public ServerBuilder gracefulShutdownExceptionFactory( - GracefulShutdownExceptionFactory toException + GracefulShutdownExceptionFactory gracefulShutdownExceptionFactory ) { - gracefulShutdownExceptionFactory = requireNonNull(toException, "toException"); + this.gracefulShutdownExceptionFactory = requireNonNull(gracefulShutdownExceptionFactory, "gracefulShutdownExceptionFactory"); return this; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
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/DefaultGracefulShutdown.javacore/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdown.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.javacore/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javacore/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.javacore/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.javacore/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandlerBuilder.javacore/src/main/java/com/linecorp/armeria/server/HttpServerHandler.javacore/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.javacore/src/main/java/com/linecorp/armeria/server/ServerConfig.javacore/src/main/java/com/linecorp/armeria/server/UpdatableServerConfig.javacore/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.javacore/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.javacore/src/test/java/com/linecorp/armeria/server/GracefulShutdownBuilderTest.javacore/src/test/java/com/linecorp/armeria/server/GracefulShutdownSupportTest.javacore/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.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/server/HttpServerPipelineConfigurator.javacore/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdown.javacore/src/main/java/com/linecorp/armeria/server/UpdatableServerConfig.javacore/src/main/java/com/linecorp/armeria/common/Flags.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/server/HttpServerHandler.javacore/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.javacore/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandlerBuilder.javacore/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.javacore/src/main/java/com/linecorp/armeria/server/ServerConfig.javacore/src/main/java/com/linecorp/armeria/common/DefaultGracefulShutdown.javacore/src/test/java/com/linecorp/armeria/server/GracefulShutdownSupportTest.javacore/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javacore/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.javacore/src/test/java/com/linecorp/armeria/server/GracefulShutdownBuilderTest.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java
🧬 Code graph analysis (8)
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
UnstableApi(28-111)
core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandlerBuilder.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/server/ServerConfig.java (2)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
UnstableApi(28-111)core/src/main/java/com/linecorp/armeria/common/ShuttingDownException.java (1)
UnstableApi(27-47)
core/src/test/java/com/linecorp/armeria/server/GracefulShutdownSupportTest.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/server/Server.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java (2)
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
Flags(106-1907)core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
UnstableApi(28-111)
⏰ 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 (26)
core/src/test/java/com/linecorp/armeria/server/GracefulShutdownSupportTest.java (1)
39-39: LGTM!The import update correctly reflects the package relocation of
GracefulShutdownSupportfromservertocommon.core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandlerBuilder.java (1)
18-18: LGTM!The import update correctly reflects the package relocation of
GracefulShutdownSupportto the common package.core/src/main/java/com/linecorp/armeria/server/HttpServerPipelineConfigurator.java (1)
49-49: LGTM!The import update correctly reflects the package relocation of
GracefulShutdownSupportto the common package.core/src/main/java/com/linecorp/armeria/server/Http2ServerConnectionHandler.java (1)
21-21: LGTM!The import update correctly reflects the package relocation of
GracefulShutdownSupportto the common package.core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java (1)
467-477: LGTM!The graceful shutdown integration is well implemented:
- Correctly retrieves the
GracefulShutdownconfiguration from options- Properly converts
Durationvalues to milliseconds- Preserves the existing error-logging behavior via the listener
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java (2)
48-48: LGTM!The import update correctly reflects the package relocation of
GracefulShutdownSupportto the common package.
333-338: LGTM!The refactoring to use the factory-based exception creation pattern is cleaner and aligns with the broader graceful shutdown configuration changes. The factory approach allows for more flexible exception customization per-context.
core/src/test/java/com/linecorp/armeria/client/ClientFactoryBuilderTest.java (2)
323-348: Client worker-group graceful shutdown test covers config and defaults wellThe
gracefulShutdown()test correctly:
- Asserts that a custom
GracefulShutdowninstance configured viaworkerGroup(1, workerGroupGracefulShutdown)is propagated intoClientFactoryOptions.- Verifies that when unset,
workerGroupGracefulShutdownandhttp2GracefulShutdownTimeoutMillisfall back to the correspondingFlagsvalues.No issues from a correctness or coverage perspective.
350-383: HTTP/2 graceful shutdown timeout tests look correct and completeThe
http2GracefulShutdownTimeoutMillis()test:
- Validates both
Durationand millisecond-based setters.- Confirms
0disables the timeout.- Ensures negative values are rejected with an
IllegalArgumentExceptionand an informative message fragment.This aligns with the builder’s validation logic and expected semantics.
core/src/main/java/com/linecorp/armeria/common/DefaultGracefulShutdown.java (1)
29-60: Value-type implementation for GracefulShutdown is consistentThe constructor, accessors,
equals, andhashCodecorrectly treatquietPeriodandtimeoutas the defining state. No functional or equality/ordering issues spotted.core/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.java (2)
824-846: gracefulShutdownExceptionFactory test matches new factory semanticsThe test:
- Verifies the default factory produces a
ShuttingDownException.- Verifies a custom factory via
gracefulShutdownExceptionFactory(...)is used instead.This is the right level of coverage for the new factory hook. Passing
nullfor(ctx, req)is acceptable as long as the factory contract allows@Nullableparameters, which seems consistent with internal usage.
848-892: Server graceful shutdown configuration test is soundThe
gracefulShutdown()test:
- Confirms that custom
GracefulShutdowninstances for server-wide, boss group, and worker group are all propagated intoServerConfig.- Validates that, when unset, all three default to
GracefulShutdown.disabled().This accurately exercises the new configuration plumbing without introducing resource-management issues (servers are not started).
core/src/test/java/com/linecorp/armeria/server/GracefulShutdownBuilderTest.java (1)
26-68: GracefulShutdownBuilder tests remain aligned with the builder contractUpdating the import to
com.linecorp.armeria.common.GracefulShutdownmatches the new package. The tests for invalid values, default (0/0), and custom durations still reflect the current builder behavior, so coverage remains appropriate.core/src/main/java/com/linecorp/armeria/server/Server.java (2)
504-622: GracefulShutdownSupport integration during start is coherent
gracefulShutdownSupportis initialized once indoStart()based onconfig().gracefulShutdown().quietPeriod(), withcreateDisabled()used for the zero-quiet-period case.- The instance is then consistently injected into
HttpServerPipelineConfiguratorand metric setup.The use of a volatile field plus local copies in methods avoids concurrency pitfalls, and the disabled vs enabled behavior is cleanly separated via
GracefulShutdownSupport.createDisabled().
624-657: Server stop now cleanly honors configurable worker/boss graceful shutdown
doStop()correctly:
- Waits for
GracefulShutdownSupport.completedQuietPeriod()when enabled, using a dedicated scheduled executor with a hard timeout driven byconfig.gracefulShutdown().timeout().- Once past the quiet period, closes server channels and accepted sockets, then:
- Shuts down the worker group only if
config.shutdownWorkerGroupOnStop()is true, usingconfig.workerGroupGracefulShutdown()’s quiet period and timeout.- Shuts down all boss groups using
config.bossGroupGracefulShutdown()’s quiet period and timeout, and waits on theirterminationFuture()s.This matches the new configuration model and preserves the previous ordering (worker before boss). I don’t see correctness or lifecycle issues here.
Also applies to: 689-727
core/src/main/java/com/linecorp/armeria/common/Flags.java (1)
464-466: New workerGroupGracefulShutdown flag follows existing flag patterns
WORKER_GROUP_GRACEFUL_SHUTDOWNis initialized viagetValue(FlagsProvider::workerGroupGracefulShutdown, ...), consistent with other flags, so SPI/DefaultFlagsProvider overrides will work as expected.workerGroupGracefulShutdown()simply returns the cached instance and is annotated@UnstableApi, which matches the new, advanced configuration nature.Javadoc stating that the default disables graceful shutdown is consistent with a default
GracefulShutdownofquietPeriod=0andtimeout=0.Also applies to: 1736-1745
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.java (1)
23-43: LGTM!The new
GracefulShutdownExceptionFactoryfunctional interface is well-designed:
- Properly annotated with
@UnstableApias required for new public APIs- Clear Javadoc explaining the purpose and return value semantics
@Nullablereturn type correctly allowsnullto fall back toShuttingDownExceptioncore/src/main/java/com/linecorp/armeria/server/ServerConfig.java (1)
266-288: LGTM!The new
ServerConfiginterface methods are well-defined:
- All three accessors (
workerGroupGracefulShutdown(),bossGroupGracefulShutdown(),gracefulShutdownExceptionFactory()) are properly annotated with@UnstableApi- Javadocs clearly describe behavior, including the null-return semantics for the exception factory
core/src/main/java/com/linecorp/armeria/server/UpdatableServerConfig.java (1)
254-267: LGTM!The delegate implementations correctly forward calls to the underlying
DefaultServerConfig, maintaining the consistent delegation pattern used throughout this class.core/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.java (1)
47-65: LGTM!The test refactoring correctly exercises the new
gracefulShutdownExceptionFactory()API:
- Conditionally configures the factory only when testing custom exceptions
- Validates both the HTTP status and the response cause type
- Covers both default (
ShuttingDownException) and custom (AnticipatedException) code pathscore/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
17-110: LGTM!The relocation to
com.linecorp.armeria.commonand API simplification are well-executed:
- Package move appropriately reflects that
GracefulShutdownis now used by both client and server components- Javadoc updates to "client or server" accurately reflect the broader scope
- The
toExceptionFunctionremoval aligns with the newGracefulShutdownExceptionFactorypattern in the server packagecore/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (1)
2516-2518: Correctly wiring new graceful shutdown parameters.The new graceful shutdown fields are properly passed to
DefaultServerConfig, enabling the full configuration flow.core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java (4)
97-99: New fields for graceful shutdown configuration.The three new fields are properly declared and will store the per-group graceful shutdown settings and exception factory.
196-199: Proper null-checking in constructor.The
requireNonNullchecks ensure the new graceful shutdown parameters are always provided, preventing potential NPEs.
608-621: LGTM!The new getter implementations correctly return the stored configuration values, following the established pattern in this class.
731-738: LGTM!The
toStringmethod updates properly include the new graceful shutdown fields for debugging visibility, following the existing formatting conventions.Also applies to: 835-838
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for customizing the graceful shutdown delay of Netty EventLoopGroups for both client and server components. It materializes proposals from issue #5813 by moving graceful shutdown configuration classes from the server package to the common package, making them reusable across the codebase.
Key changes:
- Moved
GracefulShutdown,GracefulShutdownBuilder,GracefulShutdownSupport, andDefaultGracefulShutdownfromservertocommonpackage for broader reusability - Added
GracefulShutdownExceptionFactoryinterface to separate exception generation logic from graceful shutdown configuration - Extended
ServerBuilderandClientFactoryBuilderwith new overloaded methods acceptingGracefulShutdownparameters for worker and boss groups - Added global configuration support via
FlagsandFlagsProviderfor default worker group graceful shutdown behavior
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/com/linecorp/armeria/common/GracefulShutdown.java |
Moved from server package to common package; removed server-specific toException method |
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java |
Moved from server package; updated documentation to reflect client/server usage; removed toExceptionFunction method |
core/src/main/java/com/linecorp/armeria/common/DefaultGracefulShutdown.java |
Moved from server package; removed exception function field |
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java |
Moved from server package to common; changed visibility to public |
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.java |
New interface for creating shutdown exceptions in server contexts |
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java |
Added overloaded workerGroup methods with GracefulShutdown parameter and bossGroupGracefulShutdown/gracefulShutdownExceptionFactory configuration methods |
core/src/main/java/com/linecorp/armeria/server/ServerConfig.java |
Added getter methods for worker/boss group graceful shutdown and exception factory |
core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java |
Implemented new configuration fields and updated constructor/toString |
core/src/main/java/com/linecorp/armeria/server/Server.java |
Updated shutdown logic to use configured graceful shutdown settings |
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java |
Added overloaded workerGroup methods with GracefulShutdown parameter |
core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java |
Added WORKER_GROUP_GRACEFUL_SHUTDOWN option and getter method |
core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java |
Updated to use graceful shutdown configuration when closing worker group |
core/src/main/java/com/linecorp/armeria/common/Flags.java |
Added workerGroupGracefulShutdown() method for global default configuration |
core/src/main/java/com/linecorp/armeria/common/FlagsProvider.java |
Added workerGroupGracefulShutdown() default method |
| Test files | Updated imports and tests to reflect moved classes and new API |
Comments suppressed due to low confidence (3)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdown.java:26
- The class-level documentation states "Configures the graceful shutdown behavior of a {@link Server}." However, this interface is now in the common package and is used for both server and client shutdown (as evidenced by its usage in ClientFactoryOptions and ServerConfig). The documentation should be updated to reflect that it can be used for both client and server graceful shutdown configuration.
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java:50 - The documentation states "Sets the quiet period to wait for active requests to go end before shutting down." This should be "to end" instead of "go end" for proper grammar.
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java:63 - The documentation states "Sets the quiet period millis to wait for active requests to go end before shutting down." This should be "to end" instead of "go end" for proper grammar.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (1)
576-587: Missing@UnstableApiannotation and@paramjavadoc forgracefulShutdown.This new public API method needs the
@UnstableApiannotation per coding guidelines. Additionally, thegracefulShutdownparameter is missing from the javadoc.🔎 Proposed fix
/** * Uses a newly created {@link EventLoopGroup} with the specified number of threads for * performing socket I/O and running {@link Service#serve(ServiceRequestContext, Request)}. * The worker {@link EventLoopGroup} will be shut down when the {@link Server} stops. * * @param numThreads the number of event loop threads + * @param gracefulShutdown the {@link GracefulShutdown} configuration for the worker {@link EventLoopGroup} */ + @UnstableApi public ServerBuilder workerGroup(int numThreads, GracefulShutdown gracefulShutdown) {
📜 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/HttpClientFactory.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.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/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
🧬 Code graph analysis (2)
core/src/main/java/com/linecorp/armeria/server/Server.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownSupport.java (1)
GracefulShutdownSupport(30-160)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
UnstableApi(28-111)
⏰ 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 (6)
core/src/main/java/com/linecorp/armeria/server/Server.java (3)
63-64: LGTM!The imports for
GracefulShutdownandGracefulShutdownSupportfrom the common package are correctly added to support the new configurable graceful shutdown behavior.
655-661: LGTM!The worker group shutdown now correctly uses the configurable
GracefulShutdownparameters fromconfig.workerGroupGracefulShutdown(), passingquietPeriodandtimeoutin milliseconds. This aligns with the PR objective to allow customization of the graceful shutdown delay.
680-687: LGTM!The boss group shutdown correctly retrieves the
GracefulShutdownconfiguration fromconfig.bossGroupGracefulShutdown()and applies the configuredquietPeriodandtimeoutto each boss group. The implementation is consistent with the worker group shutdown pattern.core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (3)
227-230: LGTM!The new fields are correctly initialized with
GracefulShutdown.disabled()as the default, and thegracefulShutdownExceptionFactoryhas a sensible default that returnsShuttingDownException.get().
991-999: LGTM!The
bossGroupGracefulShutdownmethod is correctly annotated with@UnstableApiand has appropriate javadoc documentation.
2522-2524: LGTM!The
buildServerConfigmethod is correctly updated to pass the new graceful shutdown configuration parameters (gracefulShutdown,workerGroupGracefulShutdown,bossGroupGracefulShutdown,gracefulShutdownExceptionFactory) to theDefaultServerConfigconstructor.
🔍 Build Scan® (commit: ec355b5) |
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Thanks for the initiative 👍 left some nit comments but overall looks good to me.
| * when the {@link ClientFactory} is closed | ||
| */ | ||
| public ClientFactoryBuilder workerGroup( | ||
| EventLoopGroup workerGroup, boolean shutdownOnClose, GracefulShutdown gracefulShutdown) { |
There was a problem hiding this comment.
Optional) What do you think of validating if shutdownOnClose == true since GracefulShutdown won't have meaning unless the workerGroup is actually closed?
e.g.
if (!shutdownOnClose) {
checkArgument(gracefulShutdown == GracefulShutdown.disabled(),
"GracefulShutdown is not supported when shutdownOnClose is false");
}|
|
||
| GracefulShutdownBuilder() {} | ||
|
|
||
| static Duration validateNonNegative(Duration duration, String fieldName) { |
There was a problem hiding this comment.
nit; can be private
| static Duration validateNonNegative(Duration duration, String fieldName) { | |
| private static Duration validateNonNegative(Duration duration, String fieldName) { |
| */ | ||
| @UnstableApi | ||
| default GracefulShutdown workerGroupGracefulShutdown() { | ||
| return GracefulShutdown.disabled(); |
There was a problem hiding this comment.
To allow users to override FlagsProvider#workerGroupGracefulShutdown using FlagsProvider#priority, what do you think of returning null here and returning the default GracefulShutdown at DefaultFlagsProvider?
| 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.
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");
}| @@ -27,17 +27,17 @@ | |||
| * Keeps track of pending requests to allow shutdown to happen after a fixed quiet period passes | |||
| * after the last pending request. | |||
| */ | |||
| abstract class GracefulShutdownSupport { | |||
| public abstract class GracefulShutdownSupport { | |||
There was a problem hiding this comment.
Question) Is there a reason this class has been moved to the common package?
If not, what do you think of reverting this change as I don't think there is a need to expose public classes that aren't directly beneficial to users?
There was a problem hiding this comment.
No, it was not. I moved all related classes to common at once. But you're right this one is used only from the server package, so it could and should be left there.
| * response. | ||
| */ | ||
| @UnstableApi | ||
| public ServerBuilder gracefulShutdownExceptionFactory( |
There was a problem hiding this comment.
I think specifying gracefulShutdownExceptionFactory only makes sense when ServerBuilder#gracefulShutdown is specified.
What do you think of adding a single overloaded method which also accepts gracefulShutdownExceptionFactory?
e.g.
/**
* Sets the {@link GracefulShutdown} configuration.
* If not set, {@link GracefulShutdown#disabled()} is used.
*/
@UnstableApi
public ServerBuilder gracefulShutdown(GracefulShutdown gracefulShutdown, GracefulShutdownExceptionFactory toException) {
this.gracefulShutdown = requireNonNull(gracefulShutdown, "gracefulShutdown");
this.toException = requireNonNull(toException, "toException");
return this;
}There was a problem hiding this comment.
Initially I thought this factory would be used when either graceful shutdown takes place (i.e. Armeria internal one, worker group, boss group). But I can't find a place that would confirm this hypothesis - so it probably only relates to internal Armeria graceful shutdown and could be combined this way. I'll rework this.
| private GracefulShutdown workerGroupGracefulShutdown = GracefulShutdown.disabled(); | ||
| private GracefulShutdown bossGroupGracefulShutdown = GracefulShutdown.disabled(); |
There was a problem hiding this comment.
Question) Would it make sense to use the default from Flags here?
e.g.
| private GracefulShutdown workerGroupGracefulShutdown = GracefulShutdown.disabled(); | |
| private GracefulShutdown bossGroupGracefulShutdown = GracefulShutdown.disabled(); | |
| private GracefulShutdown workerGroupGracefulShutdown = Flags.workerGroupGracefulShutdown(); | |
| private GracefulShutdown bossGroupGracefulShutdown = Flags.workerGroupGracefulShutdown(); |
There was a problem hiding this comment.
I can't answer this since I'm not fully familiar with the Flags purpose. I guess it would mean that by single system property I could set defaults to both groups, which could be both handy and misleading (single configuration option for two toggles). For now, I leave current implementation in place, but will hapilly change that if you insist on the change.
There was a problem hiding this comment.
I see, I'm mostly concerned with avoiding behavioral breaking changes since previously shutdownGracefully was called for the workerGroup and the future was waited on when the Server actually closed. I think it's not uncommon for user-code to have long-living tasks that may continue running after server connections have been drained.
What do you think of using the Flags value at least for workerGroupGracefulShutdown then?
…`EventLoopGroup`s Incorporation of comments from the review. Refs: line#5813
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (3)
1009-1009: Minor grammar correction needed."an {@link Throwable}" should be "a {@link Throwable}" since "Throwable" starts with a consonant sound.
🔎 Proposed fix
- * @param toException the function that returns an {@link Throwable} to terminate a pending request when + * @param toException the function that returns a {@link Throwable} to terminate a pending request when
559-584: Missing@UnstableApiannotation and parameter documentation.This new public method requires the
@UnstableApiannotation per project coding guidelines. Additionally, thegracefulShutdownparameter is missing its@paramjavadoc entry.🔎 Proposed fix
/** * Sets the worker {@link EventLoopGroup} which is responsible for performing socket I/O and running * {@link Service#serve(ServiceRequestContext, Request)}. * If not set, {@linkplain CommonPools#workerGroup() the common worker group} is used. * * @param workerGroup the worker {@link EventLoopGroup} * @param shutdownOnStop whether to shut down the worker {@link EventLoopGroup} * when the {@link Server} stops + * @param gracefulShutdown the {@link GracefulShutdown} configuration to be used when the worker + * {@link EventLoopGroup} is being shut down */ @UnstableApi public ServerBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnStop, GracefulShutdown gracefulShutdown) {
586-600: Missing@UnstableApiannotation and parameter documentation.This new public method requires the
@UnstableApiannotation per project coding guidelines. Additionally, thegracefulShutdownparameter is missing its@paramjavadoc entry.🔎 Proposed fix
/** * Uses a newly created {@link EventLoopGroup} with the specified number of threads for * performing socket I/O and running {@link Service#serve(ServiceRequestContext, Request)}. * The worker {@link EventLoopGroup} will be shut down when the {@link Server} stops. * * @param numThreads the number of event loop threads + * @param gracefulShutdown the {@link GracefulShutdown} configuration to be used when the worker + * {@link EventLoopGroup} is being shut down */ @UnstableApi public ServerBuilder workerGroup(int numThreads, GracefulShutdown gracefulShutdown) {
🧹 Nitpick comments (5)
core/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.java (2)
47-67: Consider adding a comment to explain the conditional configuration.The refactored server construction correctly supports testing both default and custom graceful shutdown exception paths. The conditional logic appropriately applies the custom exception factory only when
AnticipatedExceptionis the expected cause. However, a brief comment explaining why different configuration paths are needed would improve maintainability.💡 Suggested comment for clarity
+ // Configure graceful shutdown with custom exception factory only when testing AnticipatedException if (AnticipatedException.class.isAssignableFrom(expectedCause)) { serverBuilder.gracefulShutdown( gracefulShutdown, (ctx, req) -> new AnticipatedException() ); } else { serverBuilder.gracefulShutdown(gracefulShutdown); }
85-99: Variable naming could be more descriptive.The variables
defaultErrorandcustomErrordistinguish between using the framework's defaultShuttingDownExceptionversus a customAnticipatedException. However, both variables represent customGracefulShutdownconfigurations (with non-default timeout values), which makes the "default" label potentially misleading.💡 Consider more descriptive names
- final GracefulShutdown customError = + final GracefulShutdown withCustomException = GracefulShutdown.builder() .quietPeriod(Duration.ofMillis(500)) .timeout(Duration.ofMillis(500)) .build(); - final GracefulShutdown defaultError = + final GracefulShutdown withDefaultException = GracefulShutdown.builder() .quietPeriod(Duration.ofMillis(200)) .timeout(Duration.ofMillis(200)) .build(); return Stream.of( - Arguments.of(defaultError, ShuttingDownException.class, HttpStatus.SERVICE_UNAVAILABLE), - Arguments.of(customError, AnticipatedException.class, HttpStatus.BAD_GATEWAY)); + Arguments.of(withDefaultException, ShuttingDownException.class, HttpStatus.SERVICE_UNAVAILABLE), + Arguments.of(withCustomException, AnticipatedException.class, HttpStatus.BAD_GATEWAY));core/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java (1)
542-548: Optional: cache the default GracefulShutdown instance instead of rebuildingThe implementation of
workerGroupGracefulShutdown()is correct, but it constructs a newGracefulShutdownon every call. If this accessor is used frequently, you could avoid repeated allocations by caching a single static instance, e.g.:Optional caching tweak
- @Override - public GracefulShutdown workerGroupGracefulShutdown() { - return GracefulShutdown.builder() - .quietPeriod(Duration.ofSeconds(2L)) - .timeout(Duration.ofSeconds(15L)) - .build(); - } + private static final GracefulShutdown DEFAULT_WORKER_GROUP_GRACEFUL_SHUTDOWN = + GracefulShutdown.builder() + .quietPeriod(Duration.ofSeconds(2L)) + .timeout(Duration.ofSeconds(15L)) + .build(); + + @Override + public GracefulShutdown workerGroupGracefulShutdown() { + return DEFAULT_WORKER_GROUP_GRACEFUL_SHUTDOWN; + }Purely a micro-optimization; behavior is already fine as-is.
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java (1)
169-207: Optional: make gracefulShutdown null‑handling explicit for the shutdownOnClose == false caseThe new overloads look good overall: they’re annotated with
@UnstableApi, validategracefulShutdownwhenshutdownOnCloseis true, and guard against configuring a non‑disabled strategy whenshutdownOnCloseis false.One minor clarity tweak: when
shutdownOnCloseisfalse, anullgracefulShutdownwill currently fail thecheckArgument(gracefulShutdown == GracefulShutdown.disabled(), …)check, but the error message doesn’t clearly indicate thatnullis invalid.If you want a more explicit failure mode, you could require non‑null up front and keep the existing consistency check:
Optional null-handling refinement
@UnstableApi public ClientFactoryBuilder workerGroup( EventLoopGroup workerGroup, boolean shutdownOnClose, GracefulShutdown gracefulShutdown) { option(ClientFactoryOptions.WORKER_GROUP, requireNonNull(workerGroup, "workerGroup")); option(ClientFactoryOptions.SHUTDOWN_WORKER_GROUP_ON_CLOSE, shutdownOnClose); - if (shutdownOnClose) { - option(ClientFactoryOptions.WORKER_GROUP_GRACEFUL_SHUTDOWN, - requireNonNull(gracefulShutdown, "gracefulShutdown")); - } else { + gracefulShutdown = requireNonNull(gracefulShutdown, "gracefulShutdown"); + if (shutdownOnClose) { + option(ClientFactoryOptions.WORKER_GROUP_GRACEFUL_SHUTDOWN, gracefulShutdown); + } else { // disabled graceful shutdown is default, therefore we check developer didn't set anything else // because it wouldn't be used and might indicate a mistake checkArgument(gracefulShutdown == GracefulShutdown.disabled(), "GracefulShutdown makes no sense when shutdownOnClose is false"); } return this; }Behavior is already correct; this just tightens the API contract for callers.
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.java (1)
32-45: Public GracefulShutdownSupport API is coherent; consider optional validation for external callersMaking
GracefulShutdownSupportpublic with@UnstableApi, adding thecreate(...)/createDisabled()factories, and exposing the core lifecycle methods (inc,dec,pendingResponses,isShuttingDown,completedQuietPeriod) gives a consistent, testable surface without changing the underlying logic.One optional hardening idea:
create(Duration quietPeriod, ...)currently accepts anyDuration, including negative values, which would result in a quiet period that is effectively always “completed.” If you expect external callers to use these factories directly (rather than going throughGracefulShutdown/GracefulShutdownBuilder), you could reuse the same non‑negative validation as inGracefulShutdownBuilder:Optional validation tweak
- public static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor) { - return create(quietPeriod, blockingTaskExecutor, Ticker.systemTicker()); + public static GracefulShutdownSupport create(Duration quietPeriod, Executor blockingTaskExecutor) { + requireNonNull(quietPeriod, "quietPeriod"); + // mirror GracefulShutdownBuilder's non-negative requirement + if (quietPeriod.isNegative()) { + throw new IllegalArgumentException("quietPeriod: " + quietPeriod + " (expected: >= 0)"); + } + return create(quietPeriod, blockingTaskExecutor, Ticker.systemTicker()); }If you’re confident all current usages already pass validated durations, this can be deferred.
Also applies to: 49-68, 73-79, 80-91, 96-147
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.javacore/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.javacore/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.javacore/src/main/java/com/linecorp/armeria/server/HttpServerHandler.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.javacore/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.javacore/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/linecorp/armeria/server/ServerBuilderTest.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/server/GracefulShutdownExceptionFactory.javacore/src/main/java/com/linecorp/armeria/server/Server.javacore/src/main/java/com/linecorp/armeria/server/ServerBuilder.javacore/src/main/java/com/linecorp/armeria/common/FlagsProvider.javacore/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.javacore/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.javacore/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.javacore/src/main/java/com/linecorp/armeria/server/HttpServerHandler.javacore/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.javacore/src/main/java/com/linecorp/armeria/common/DefaultFlagsProvider.java
🧬 Code graph analysis (1)
core/src/main/java/com/linecorp/armeria/server/GracefulShutdownSupport.java (1)
core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
UnstableApi(28-111)
⏰ 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 (5)
core/src/test/java/com/linecorp/armeria/server/CustomGracefulShutDownTest.java (1)
34-34: LGTM: Import reflects API relocation.The import correctly uses the relocated
GracefulShutdownclass from the common package, aligning with the PR's objective to share this API between client and server components.core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java (1)
325-337: GracefulShutdownExceptionFactory integration and logging look correctThe new use of
config.gracefulShutdownExceptionFactory().createThrowableForContext(ctx, req)and the updated warning message cleanly switch over from the old API while keeping failures contained and falling back to the default shutdown cause. No changes needed here.core/src/main/java/com/linecorp/armeria/server/Server.java (1)
655-686: GracefulShutdown configuration is correctly applied to worker and boss groupsReading
GracefulShutdownfromconfigand passing its quiet period and timeout intoshutdownGracefully(...)for both worker and boss groups fits the new API and keeps externally managed worker groups untouched whenshutdownWorkerGroupOnStop()is false. The shutdown sequencing and termination listeners remain consistent with the previous behavior.core/src/main/java/com/linecorp/armeria/server/GracefulShutdownExceptionFactory.java (1)
1-43: GracefulShutdownExceptionFactory API is well-scoped and matches server usageThe new
@UnstableApifunctional interface cleanly encapsulates shutdown-specific exception creation with a nullable return, and its contract aligns with howHttpServerHandler.shutdownError()consumes it. No changes needed here.core/src/main/java/com/linecorp/armeria/common/GracefulShutdownBuilder.java (1)
42-47: GracefulShutdownBuilder validation and semantics look solidThe new
validateNonNegative(...)helper, updated Javadoc, and the simplifiedDefaultGracefulShutdown(quietPeriod, timeout)construction together provide clear semantics: non‑negative durations only, withtimeout >= quietPeriod, and support for both client and server use cases. No further changes needed here.Also applies to: 49-53, 62-65, 72-76, 85-88, 107-110
…`EventLoopGroup`s Fixed Javadoc Refs: line#5813
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Looks good overall to me - left some minor comments
| * {@link EventLoopGroup} is being shut down, makes sense only if {@link #shutdownWorkerGroupOnStop} is true | ||
| */ | ||
| @UnstableApi | ||
| public ServerBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnStop, GracefulShutdown gracefulShutdown) { |
There was a problem hiding this comment.
On second thought, it probably makes sense that shutdownOnStop isn't accepted at all since setting gracefulShutdown implies that the workerGroup will be shutdown. (it may be nice to indicate this in the javadocs though) Let me know what you think.
| public ServerBuilder workerGroup(EventLoopGroup workerGroup, boolean shutdownOnStop, GracefulShutdown gracefulShutdown) { | |
| public ServerBuilder workerGroup(EventLoopGroup workerGroup, GracefulShutdown gracefulShutdown) { |
Ditto for the equivalent method in ClientFactoryBuilder
| */ | ||
| abstract class GracefulShutdownSupport { | ||
| @UnstableApi | ||
| public abstract class GracefulShutdownSupport { |
There was a problem hiding this comment.
Do you mind also reverting the public modifiers in this class as well? I guess it's fine to just completely revert the changes in this class
There was a problem hiding this comment.
Of course, I should have reverted this automatically yesterday. My bad.
| */ | ||
| @Nullable | ||
| @UnstableApi | ||
| default GracefulShutdown workerGroupGracefulShutdown() { |
There was a problem hiding this comment.
Note to other reviewers: I think it's fine to skip the SystemPropertyFlagsProvider implementation since we will need to define a schema for parsing GracefulShutdown which I think is out of scope of this PR.
| private GracefulShutdown workerGroupGracefulShutdown = GracefulShutdown.disabled(); | ||
| private GracefulShutdown bossGroupGracefulShutdown = GracefulShutdown.disabled(); |
There was a problem hiding this comment.
I see, I'm mostly concerned with avoiding behavioral breaking changes since previously shutdownGracefully was called for the workerGroup and the future was waited on when the Server actually closed. I think it's not uncommon for user-code to have long-living tasks that may continue running after server connections have been drained.
What do you think of using the Flags value at least for workerGroupGracefulShutdown then?
…`EventLoopGroup`s Reverted changes since the GracefulShutdownSupport was not needed to be moved and its API opened. Refs: line#5813
…`EventLoopGroup`s Reusing settings from worker group graceful shutdown from flags (and thus environment settings). Refs: line#5813
…`EventLoopGroup`s Removed unnecessary (implied) parameter. Refs: line#5813
…`EventLoopGroup`s Removed unnecessary (implied) parameter. Refs: line#5813
|
@jrhee17 I changed the So this round of review comments should be sorted out. |
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Changes look good to me, thanks for your patience 👍
| ctx, config.gracefulShutdown(), e); | ||
| logger.warn( | ||
| "{} Unexpected exception from gracefulShutdownExceptionFactory.createThrowableForContext(): " | ||
| + "{}", ctx, config.gracefulShutdown(), e); |
There was a problem hiding this comment.
nit; since the exception factory is no longer tied with gracefulShutdown
| + "{}", ctx, config.gracefulShutdown(), e); | |
| + "{}", ctx, config.gracefulShutdownExceptionFactory(), e); |
…`EventLoopGroup`s Fixed log parameter that better relates to the probable issue. Refs: line#5813
| */ | ||
|
|
||
| package com.linecorp.armeria.server; | ||
| package com.linecorp.armeria.common; |
There was a problem hiding this comment.
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?
We also need to convert the old GracefulShutdown to the new one in the ServerBuilder:
@UnstableApi
public ServerBuilder gracefulShutdown(com.linecorp.armeria.server.GracefulShutdown gracefulShutdown) {
// convert to common.GracefulShutdown and GracefulShutdownExceptionFactory
return this;
}
There was a problem hiding this comment.
The original proposal was to move the class and incur a breaking change:
- I wasn't able to find many (if any) cases where the
server.GracefulShutdownconstruct was actually used - My thought was there wouldn't be many servers per application/repository - since
server.GracefulShutdownis usually configured once per server I didn't think the breaking change would affect multiple points within each codebase.
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
There was a problem hiding this comment.
After a team discussion, we have decided to introduce a breaking change for the following reasons:
- The feature was recently introduced and, based on a GitHub search, has limited adoption.
- Maintaining two separate GracefulShutdown implementations is confusing for users and adds unnecessary complexity.
@novoj Sorry for confusing you. 😉
There was a problem hiding this comment.
It's okay. I didn't react because I anticipated there might be a discussion about it. Both approaches have their benefits and disadvantages.
| */ | ||
| @UnstableApi | ||
| @FunctionalInterface | ||
| public interface GracefulShutdownExceptionFactory { |
There was a problem hiding this comment.
Just ShutdownExceptionFactory?
| * throwable should be thrown in this context | ||
| */ | ||
| @Nullable | ||
| Throwable createThrowableForContext(ServiceRequestContext serviceRequestContext, HttpRequest httpRequest); |
|
I think the current API has a few issues:
As an alternative, without changing the existing server/client builders, what if we provide factory methods in EventLoopGroups
.builder()
.numThreads(...)
.gracefulShutdown(quietPeriod, timeout)
...
.build();One downside of this approach is that armeria/core/src/main/java/com/linecorp/armeria/server/Server.java Lines 517 to 521 in c62f5e9 We could fix this by provide an extension point in ServerBuilder to create bossGroup.
Server
.builder()
.bossGroupFactory(bossThreadName -> {
EventLoopGroups.newEventLoopGroup(1, ...);
});Sorry for changing the overall direction, but I believe this approach will be more flexible. |
Adding more context, I didn't want to make you go through an entire rewrite of the current changeset - hence I volunteered to apply the changes explained by @ikhoon. You're more than welcome to continue work based on the direction proposed by @ikhoon though, let me know what you think. |
|
I could try implementing the third approach, but I'm unsure about your idea of baking the quiet period and timeout inside the EventLoopGroup. EventLoopGroup creation ends in Perhaps you had a completely different approach in mind that I'm missing? What is your opinion on this matter? I'd need more guidance to continue on this issue. |
Right, this matches what I was imagining as well.
It seems like the contract of
|
|
I understand. I can create a third implementation that follows this new plan. However, it will probably take me more than a week since the last two pull requests were made during my vacation, and now I only have weekends and evenings, which are rather scarce. So I'd suggest removing a planned milestone and assign new one when I'm finished. |
|
Yeah, we don't have to hurry. Thanks a lot, @novoj, for your work. 🙇 |
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:
This pull request is continuation of a discussion in less ambitious PR #6528 and tries to materialize @jrhee17 proposals.
Modifications:
Graceful shutdown support for client worker groups:
WORKER_GROUP_GRACEFUL_SHUTDOWNoption toClientFactoryOptions, enabling configuration of graceful shutdown for the workerEventLoopGroupwhen theClientFactoryis closed. Includes a new getter method and integration with the shutdown process. [1] [2] [3]ClientFactoryBuilderwith overloadedworkerGroupmethods that accept aGracefulShutdownparameter, allowing users to customize shutdown behavior when creating client factories.Refactoring and generalization of graceful shutdown classes:
GracefulShutdown,GracefulShutdownBuilder,GracefulShutdownSupport, andDefaultGracefulShutdownfrom the server package to the common package, making them usable for both server and client. Removed server-specific logic (such as exception handling for pending requests) to generalize the API. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Configuration and flag enhancements:
FlagsandFlagsProviderclasses, with a new flag and default implementation. [1] [2] [3]Dependency and import updates:
Result:
EventLoopGroups #5813toExceptionin GradefulShutdown was elevated asgracefulShutdownExceptionFactoryServerConfig/ServerBuilder (and wrapped in specific interface)