Code review of Kroxylicious PR #3522: feat(runtime): make Netty shutdown durations configurable

pr-3522-review.md

Review of PR #3522: feat(runtime): make Netty shutdown durations configurable

Critical Issues (must fix)

1. Compilation failure in NetworkDefinitionBuilder -- constructor arity mismatch

The CI confirms this. NetworkDefinitionBuilder.java line 50 constructs NettySettings with 4 arguments, but the PR also changes NettySettings to have 6 fields (adding shutdownQuietPeriod and shutdownTimeout). The call:

return new NettySettings(
        Optional.ofNullable(workerThreadCount),
        Optional.ofNullable(shutdownQuietPeriod).map(s -> (int) parseDuration(s).toSeconds()),
        Optional.ofNullable(authenticatedIdleTimeout).map(NetworkDefinitionBuilder::parseDuration),
        Optional.ofNullable(unauthenticatedIdleTimeout).map(NetworkDefinitionBuilder::parseDuration));

needs the two new Optional<Duration> parameters (shutdownQuietPeriod, shutdownTimeout). The fix should also address the next issue.

2. Operator still maps to the deprecated shutdownQuietPeriodSeconds with truncation

The entire motivation of this PR is to avoid sub-second truncation, yet NetworkDefinitionBuilder maps the CRD's shutdownQuietPeriod string to the deprecated shutdownQuietPeriodSeconds integer field via (int) parseDuration(s).toSeconds(). This reintroduces the very bug the PR fixes. The operator should populate the new shutdownQuietPeriod Duration field instead. The tests in NetworkDefinitionBuilderTest and KafkaProxyReconcilerIT all assert against shutdownQuietPeriodSeconds rather than the new field, which would also need updating.

3. CRD change is a public API change

Adding a network section to the KafkaProxy CRD in kroxylicious-kubernetes-api is a public API change. Per the project's API-vs-implementation policy, this should go through the proposal process. The PR description and linked issue don't mention a proposal. This needs to be flagged and either a proposal raised, or confirmation obtained that one already exists.

Important Issues (should fix)

4. shutdownTimeout missing from CRD

The runtime NettySettings now supports shutdownTimeout, and the CHANGELOG advertises it, but the CRD has no shutdownTimeout field for either proxy or management. Operator users cannot configure this. If the CRD is being extended in this PR, shutdownTimeout should be included for completeness, or the omission should be explicitly documented as intentional.

5. Missing class-level Javadoc on NetworkDefinitionBuilder

The class declaration at line 174 of the diff:

final class NetworkDefinitionBuilder {

has no Javadoc. Project conventions require a class-level Javadoc explaining purpose.

6. Missing newline at end of file in multiple new files

The diff shows \ No newline at end of file for:

• NetworkDefinitionBuilder.java
• NetworkDefinitionBuilderTest.java
• invalid-network-proxy-shutdownQuietPeriod-bad-format.yaml
• invalid-network-proxy-workerThreadCount-below-minimum.yaml
• valid-network-settings.yaml

7. NettySettings record field ordering is awkward

The new field order is:

workerThreadCount, shutdownQuietPeriodSeconds, shutdownQuietPeriod, shutdownTimeout,
authenticatedIdleTimeout, unauthenticatedIdleTimeout

Having the deprecated field sandwiched between other active fields is confusing. Consider moving shutdownQuietPeriodSeconds to the end of the parameter list. However, this changes the canonical constructor signature, so any existing YAML configurations or code constructing NettySettings would need updating. Given that this record lives in kroxylicious-runtime and is not public API, this is feasible but may not be worth the churn.

8. CHANGELOG typo

Line 10: "adds support sub-second precision" should be "adds support for sub-second precision".

9. Deprecation warning logged on every resolve, not once

In EventGroupConfig.resolveQuietPeriod(), the deprecation warning is logged via:

STARTUP_SHUTDOWN_LOGGER.warn(
    "shutdownQuietPeriodSeconds is deprecated, use shutdownQuietPeriod ...");

This is called in build(), which runs once per event group (proxy + management), so the warning may appear twice. Minor, but worth noting.

Suggestions (consider)

10. Test for sub-second quiet period in the operator path

The NetworkDefinitionBuilderTest.shouldParseShutdownQuietPeriodToSeconds parameterised test only uses whole-second and larger values. Adding a sub-second case (e.g. "500ms") would exercise the truncation path and demonstrate the bug (currently the operator truncates to 0).

11. Duration regex in CRD allows 0s, 0ms etc.

The CRD pattern ^([0-9]+(d|h|m|s|ms|us|μs|ns))+$ allows zero-value durations like "0s" for quiet period and timeouts. Consider whether a zero quiet period or zero timeout is actually valid for Netty shutdown. Netty does accept 0, but it effectively disables the quiet period, which may surprise users.

12. CRD allows d (days) for shutdown quiet period

A shutdown quiet period of "1d" would be 86400 seconds, which is almost certainly a misconfiguration. Consider whether the pattern should be restricted, or whether validation/range checking should be added.

13. Consider using @Deprecated(since = "...", forRemoval = true) on shutdownQuietPeriodSeconds

The @Deprecated annotation on the record field could be enriched with since and forRemoval to give clearer signal about the deprecation timeline mentioned in the PR description ("eligible for removal in 3 minor releases").

Positive Observations

• The decision to pre-resolve shutdown durations in EventGroupConfig.build() and store them as record fields is clean. It keeps shutdownGracefully() zero-arg and separates configuration resolution from usage.
• Using nanosecond precision with Duration.toNanos() when calling Netty's shutdownGracefully() correctly preserves sub-second precision.
• The deprecated field fallback chain (shutdownQuietPeriod -> shutdownQuietPeriodSeconds -> default) with a deprecation warning is a reasonable migration strategy.
• The DurationSerde ordering fix in ProxyConfigStateData (registering it after JavaTimeModule) is well-commented and correctly addresses the module override issue.
• Good test coverage on the runtime side, including the parameterised shutdown duration tests and the mock-based shutdownGracefully verification test.