Repository navigation
Add proposal for configurable upstream connect timeout (#4327) - #137
Merged
Merged
Conversation
AdityaThakur1998
force-pushed
the
design-proposal-#4327
branch
from
September 14, 2026 17:48
4a871cf to
fd3ae22
Compare
AdityaThakur1998
marked this pull request as ready for review
September 14, 2026 17:49
MarkNSweep
reviewed
Sep 15, 2026
Proposes a connectTimeout key on ClusterDefinition to replace Netty's invisible 30s default with an operator-controlled, 10s-default bound on upstream TCP connects. Assisted-By: Claude Sonnet 5 <[email protected]> Signed-off-by: AdityaThakur1998 <[email protected]>
AdityaThakur1998
force-pushed
the
design-proposal-#4327
branch
from
September 23, 2026 11:40
fd3ae22 to
eb68702
Compare
…0s default PR comments flagged that existing deployments may depend on the current 30-second bound in slow-network environments, and that some test environments need it raised rather than lowered. Accept the feedback and make the whole proposal additive-only: - Netty's 30s default is retained; connectTimeout only changes behaviour when explicitly set, in either direction. - Motivation now covers both directions: shortening the wait against a blackholed address, or lengthening it past 30s for slow links/test environments. - Reworked the SYN-retransmission reasoning: Linux's schedule runs to 15s, not just 1s/3s, which rules out a flat 10s default rather than justifying one. - Compatibility collapses to a single additive-key paragraph; the targetCluster no-opt-out caveat no longer applies. - Rejected alternatives: replaced "leave the default and only add configurability" (now the accepted design) with "lower the default" (rejected). Update the proposal now that the ClusterDefinition selection-strategy work has landed: - Rename resolveSelectionStrategy()/resolveConnectTimeout() references to the landed effectiveSelectionStrategy() and the matching proposed effectiveConnectTimeout(). - Tighten wording on where resolution lives: ClusterDefinition holds and hands off the field, only TargetCluster resolves it. - Separate the two precedents cited for the compact constructor: #4840 (PR #4891) for threading a nullable component through toTargetCluster(), NettySettings for rejecting a negative duration. - Update the #4840 reference from unmerged to landed. Signed-off-by: AdityaThakur1998 <[email protected]> Assisted-By: Claude Sonnet 5 <[email protected]>
AdityaThakur1998
force-pushed
the
design-proposal-#4327
branch
from
September 23, 2026 21:42
eb68702 to
466765e
Compare
MarkNSweep
reviewed
Sep 30, 2026
MarkNSweep
left a comment
MarkNSweep
left a comment
There was a problem hiding this comment.
Just two comments looking to avoid the proposal going stale as the code base evolves.
k-wall
reviewed
Oct 1, 2026
k-wall
approved these changes
Oct 1, 2026
k-wall
left a comment
k-wall
left a comment
Member
There was a problem hiding this comment.
LGTM.
Non-blocking feedback - I think this proposal is a bit lengthy. I think you could cut quite a lot of text from the proposal without sacrificing the important points - the public API and the behaviour of the client/proxy/server as a whole. LLMs can be really verbose - that costs reviewers time.
Contributor
Author
Thanks for the feedback everyone. I also think that proposal is a bit lengthy. I really need to configure output style of Claude code for design proposals |
robobario
approved these changes
Oct 2, 2026
robobario
left a comment
robobario
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks @AdityaThakur1998
- Rewrite proposal for conciseness without losing meaning - Remove trailing cross-reference in rejected-alternatives section Signed-off-by: AdityaThakur1998 <[email protected]> Assisted-By: Claude Sonnet 4.6 (1M context) <[email protected]>
k-wall
reviewed
Oct 5, 2026
| one already in that suite. | ||
| The existing `ResilienceIT` bootstrap test does not cover this path: its fake servers accept the TCP | ||
| connection before closing it, so the connect succeeds and the timeout is never reached. A regression | ||
| test needs an address that accepts no connection at all — a non-routable or firewalled address — |
k-wall
pushed a commit
to k-wall/design
that referenced
this pull request
Oct 9, 2026
…licious#137) * Add proposal for configurable upstream connect timeout (#4327) Proposes a connectTimeout key on ClusterDefinition to replace Netty's invisible 30s default with an operator-controlled, 10s-default bound on upstream TCP connects. Assisted-By: Claude Sonnet 5 <[email protected]> Signed-off-by: AdityaThakur1998 <[email protected]> * Revise upstream connect timeout proposal per review: retain Netty's 30s default PR comments flagged that existing deployments may depend on the current 30-second bound in slow-network environments, and that some test environments need it raised rather than lowered. Accept the feedback and make the whole proposal additive-only: - Netty's 30s default is retained; connectTimeout only changes behaviour when explicitly set, in either direction. - Motivation now covers both directions: shortening the wait against a blackholed address, or lengthening it past 30s for slow links/test environments. - Reworked the SYN-retransmission reasoning: Linux's schedule runs to 15s, not just 1s/3s, which rules out a flat 10s default rather than justifying one. - Compatibility collapses to a single additive-key paragraph; the targetCluster no-opt-out caveat no longer applies. - Rejected alternatives: replaced "leave the default and only add configurability" (now the accepted design) with "lower the default" (rejected). Update the proposal now that the ClusterDefinition selection-strategy work has landed: - Rename resolveSelectionStrategy()/resolveConnectTimeout() references to the landed effectiveSelectionStrategy() and the matching proposed effectiveConnectTimeout(). - Tighten wording on where resolution lives: ClusterDefinition holds and hands off the field, only TargetCluster resolves it. - Separate the two precedents cited for the compact constructor: #4840 (PR #4891) for threading a nullable component through toTargetCluster(), NettySettings for rejecting a negative duration. - Update the #4840 reference from unmerged to landed. Signed-off-by: AdityaThakur1998 <[email protected]> Assisted-By: Claude Sonnet 5 <[email protected]> * Revise upstream connect timeout proposal: address review comments - Rewrite proposal for conciseness without losing meaning - Remove trailing cross-reference in rejected-alternatives section Signed-off-by: AdityaThakur1998 <[email protected]> Assisted-By: Claude Sonnet 4.6 (1M context) <[email protected]> --------- Signed-off-by: AdityaThakur1998 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design proposal for kroxylicious/kroxylicious#4327 — making the upstream connect
timeout configurable.
What it proposes
A
connectTimeoutkey onClusterDefinition, carried intoTargetClusterandapplied to the proxy's upstream connections. Netty's existing 30-second default
is retained when the key is unset, so the change is purely additive.
Kroxylicious sets no connect timeout today, so Netty's own default of 30 seconds
governs every deployment, invisibly and with no way to change it. The proposal is
about taking control of a bound already in force rather than introducing one.
Why it matters
An upstream address that silently drops packets — a firewall or security group
configured to DROP, a dead host whose address still routes — costs the full 30
seconds. A refused connection costs about one round trip, which is why this has
gone unnoticed.
The proxy dials one address per downstream connection and does not retry, so
progress past a bad address depends on the client reconnecting. Measured with an
AdminClient on default timeouts: one bad address ahead of a working broker
succeeded in 30.7s; three failed outright at 60.4s, the client's own budget
exhausted before the round robin reached a broker that would have answered. At a
2-second timeout the same cases took 2.7s and 7.1s, both succeeding.
The key serves the opposite need too — operators on high-latency links or in test
environments have hit the reverse problem, where 30 seconds is too short.
Points reviewers may want to weigh in on
connectTimeoutis provisional.jitter are non-goals. Intra-connection failover across the bootstrap list is
listed as a future extension and probably wants its own issue.
targetClusterform, which isslated for removal under #4462 in the same release.
Assisted-by: Claude-Opus-5 [email protected]