Skip to content

[rpc] Avoid retaining failed connections in the connection pool - #4884

Open
beryllw wants to merge 5 commits into
apache:mainfrom
beryllw:codex/fix-netty-connection-registration
Open

beryllw wants to merge 5 commits into
apache:mainfrom
beryllw:codex/fix-netty-connection-registration

Conversation

@beryllw

@beryllw beryllw commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Closes #4904.

An immediate connection failure on a Netty event loop can invoke the removal callback inside connections.computeIfAbsent(). Recursive removal fails, leaving a disconnected connection cached and preventing subsequent requests from reconnecting.

Brief change log

  • Defer connection removal to the client's event-loop group, preserving the endpoint key and identity-checked removal. An immediate retry may still fail before cleanup runs; subsequent retries can create a new connection.
  • Cover immediate DNS failure, eventual cleanup and successful reconnection with the same client using one and four network threads, with bounded asynchronous waits.
  • Cover Admin retries when either the host or port changes while the server UID remains unchanged, reusing the shared test client already merged in [test] Replace Mockito in metadata bootstrap tests #4935.

Tests

  • After rebasing onto [test] Replace Mockito in metadata bootstrap tests #4935: FlussAdminTest, MetadataUpdaterTest, NettyClientTest and RpcGatewayManagerTest passed (38 cases), including compilation of the client and its dependencies.
  • The reconnect regression was previously verified to fail for both network-thread variants when deferred connection removal was reverted.
  • Formatting, Checkstyle and license checks passed.

API and Format

No public API, RPC protocol or storage format changes.

Documentation

Not required.

Generative AI disclosure:

  • Yes — OpenAI Codex.

@beryllw beryllw changed the title [rpc] Avoid caching connections closed during construction [rpc] Avoid retaining failed connections in the connection pool Oct 27, 2026
@beryllw
beryllw requested a review from loserwang1024 October 27, 2026 05:57
@beryllw
beryllw marked this pull request as ready for review October 27, 2026 05:57
@beryllw
beryllw force-pushed the codex/fix-netty-connection-registration branch from f911252 to b05d57d Compare October 29, 2026 06:00
@beryllw
beryllw requested a review from swuferhong October 29, 2026 06:02
@beryllw

beryllw commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

@loserwang1024 @swuferhong cc

@beryllw
beryllw marked this pull request as draft October 29, 2026 06:20
@beryllw
beryllw marked this pull request as ready for review October 29, 2026 07:15
beryllw and others added 5 commits October 29, 2026 16:11
Register connections before starting an idempotent connect operation so
immediate connection failures can remove the cached entry safely.

Add regressions for immediate address resolution failure followed by
reconnection and for concurrent requests sharing one connection.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 64/64
AI-Contributed/UT: 114/114
Remove the test-only auto-connect constructor and start connections
explicitly in existing tests. Reuse the single-loop client fixture,
simplify the resolver stub, and trim redundant assertions and comments.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 14/14
AI-Contributed/UT: 38/38
Queue connection eviction to avoid re-entering computeIfAbsent on an
immediate connection failure. Keep conditional removal and the recovery
regression, without changing ServerConnection initialization.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 51/51
AI-Contributed/UT: 42/42
Run the immediate DNS failure regression with one and four network threads.
Keep connection establishment on the caller's event loop and wait for
asynchronous cleanup before verifying recovery with the same client.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 0/0
AI-Contributed/UT: 18/18
Cover admin retries after host or port changes while the server UID stays
unchanged, using the shared RPC test client from main. Keep network-thread
configuration local to the reconnect regression and bound its waits.

Co-Authored-By: Codex <noreply@openai.com>
AI-Model: gpt-6
AI-Contributed/Feature: 0/0
AI-Contributed/UT: 36/36
@beryllw
beryllw force-pushed the codex/fix-netty-connection-registration branch from d137859 to e0594f5 Compare October 29, 2026 08:16
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.

[rpc] Immediate connection failure can leave a disconnected connection cached

1 participant