Skip to content

[client] Introduce dedicated client.lookup.close-timeout config option#3774

Open
XuQianJin-Stars wants to merge 1 commit into
apache:mainfrom
XuQianJin-Stars:feat/lookup-client-close-timeout
Open

[client] Introduce dedicated client.lookup.close-timeout config option#3774
XuQianJin-Stars wants to merge 1 commit into
apache:mainfrom
XuQianJin-Stars:feat/lookup-client-close-timeout

Conversation

@XuQianJin-Stars

Copy link
Copy Markdown
Contributor

Purpose

LookupClient#close(Duration timeout) already accepts a Duration parameter,
but the only caller in FlussConnection#close() was hard-coding
Duration.ofMillis(Long.MAX_VALUE) to let pending lookup requests drain.
That hard-coded value is undesirable in K8s / graceful-shutdown scenarios
where the client must finish within a bounded budget (e.g. before SIGTERM
SIGKILL grace period elapses), and it is also harder to tune per-deployment.

This PR introduces a dedicated, user-tunable config option
client.lookup.close-timeout and wires it through the only existing caller,
while keeping the public LookupClient#close(Duration) signature intact so
that no downstream code is broken.

Linked issue: N/A (usability / K8s shutdown observability improvement)

Brief change log

  • Add new ConfigOption<Duration> CLIENT_LOOKUP_CLOSE_TIMEOUT
    (client.lookup.close-timeout) in
    fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java,
    with default value Long.MAX_VALUE (preserves previous behavior) and a
    javadoc-style description explaining the K8s / SIGTERM use case.
  • Update FlussConnection#close() in
    fluss-client/src/main/java/org/apache/fluss/client/FlussConnection.java:
    read the new option from the connection's Configuration and pass it to
    lookupClient.close(...) instead of the hard-coded
    Duration.ofMillis(Long.MAX_VALUE). The previous comment is updated to
    reflect the new behavior.
  • No change to LookupClient#close(Duration) itself — its public
    signature is preserved, so existing direct callers keep working.
  • Add new test class
    fluss-client/src/test/java/org/apache/fluss/client/lookup/LookupClientCloseTest.java
    covering 5 close-timeout scenarios (see Tests).

Tests

  • LookupClientCloseTest#testCloseWithVeryShortTimeoutReturnsNormally — closes
    a fresh LookupClient with Duration.ofMillis(1); asserts no exception is
    thrown and no InterruptedException leaks.
  • LookupClientCloseTest#testCloseWithZeroTimeoutReturnsNormally — closes
    with Duration.ZERO; verifies the API tolerates a zero-duration close.
  • LookupClientCloseTest#testCloseWithLargeTimeoutReturnsNormally — closes
    with Duration.ofSeconds(5); sanity check for the normal happy-path
    duration.
  • LookupClientCloseTest#testCloseIsIdempotent — closes the same client
    twice in a row; verifies the second call also returns without error.
  • LookupClientCloseTest#testDefaultCloseTimeoutConfigValue — asserts the
    default value of CLIENT_LOOKUP_CLOSE_TIMEOUT is
    Duration.ofMillis(Long.MAX_VALUE), locking in backwards compatibility.

All 5 tests are unit tests (no real cluster required); they build a
LookupClient via a TestingMetadataUpdater. Pre-existing
fluss-client tests are unaffected — no regression observed locally.

API and Format

  • No public API removed or renamed. LookupClient#close(Duration) keeps
    the same signature.
  • New config option follows the
    AGENTS.md §8 Configuration Patterns: durationType(),
    explicit defaultValue(...), javadoc withDescription(...),
    hierarchical key client.lookup.close-timeout.
  • mvn spotless:apply clean.
  • Java 8 compatible: no var, no Optional.isEmpty, no List.of.
  • All test assertions use AssertJ (assertThat(...).isEqualTo(...) /
    assertThatCode(...).doesNotThrowAnyException()).
  • Commit title [client] Introduce dedicated client.lookup.close-timeout config option follows the <component> prefix convention.
  • ./mvnw clean install -DskipTests -T 32 passes locally on JDK
    11.0.23-kona.

Documentation

  • The new option's javadoc / withDescription already documents the K8s
    SIGTERM use case, so it will be picked up by the doc generator
    (./mvnw -pl fluss-docgen verify) automatically.
  • No changes to the public website are needed beyond the auto-generated
    config reference entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant