Broker startup pre-connect: adaptive straggler-grace window

[jineshparakh]

PR flow

Adaptive straggler-grace window scales wait time after first connect to twice its latency, preventing premature timeout when channels complete in waves.

flowchart TD
  N0["Initialize graceMs #61; STRAGGLER#95;GRACE#95;MS #40;F1#41;"]:::stAdded
  N1["Loop over each ChannelTarget #40;F1#41;"]:::stUnchanged
  N2["Compute waitMs#58; if connected#61;#61;0 then remainingMs else min#40;remainingMs#44; graceMs#41; #40;F1#41;"]:::stModified
  N3["Poll completionService with waitMs #40;F1#41;"]:::stModified
  N4["Check if future #61;#61; null #40;timeout#41; #40;F1#41;"]:::stModified
  N5["Break loop on timeout #40;release early#41; #40;F1#41;"]:::stModified
  N6["Process successful connect #40;future#46;get#40;#41; true#41; #40;F1#41;"]:::stModified
  N7["First#45;success#58; set graceMs #61; max#40;STRAGGLER#95;GRACE#95;MS#44; 2 #42; elapsed#41; #40;F1#41;"]:::stAdded
  N8["Increment connected count #40;F1#41;"]:::stModified
  N9["Continue to next loop iteration #40;F1#41;"]:::stUnchanged
  N0 -->|"start loop"| N1
  N1 -->|"enter iteration"| N2
  N2 -->|"use waitMs"| N3
  N3 -->|"poll result"| N4
  N4 -->|"future null #45;#62; break"| N5
  N4 -->|"future not null #45;#62; handle"| N6
  N6 -->|"connected#61;#61;0 #45;#62; first success"| N7
  N7 -->|"after graceMs update"| N8
  N6 -->|"not first success #45;#62; increment"| N8
  N8 -->|"post#45;increment"| N9
  N9 -->|"next iteration"| N1
  classDef stAdded fill:#dafbe1,stroke:#1a7f37,color:#1f2328,stroke-width:2px
  classDef stModified fill:#fff8c5,stroke:#9a6700,color:#1f2328,stroke-width:2px
  classDef stRemoved fill:#ffebe9,stroke:#cf222e,color:#1f2328,stroke-width:2px
  classDef stUnchanged fill:#f6f8fa,stroke:#656d76,color:#1f2328,stroke-width:1px

Loading

AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing

Diff evidence

• F1: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/ServerPreConnector.java — before · after

• Regenerate PR flow

Broker startup pre-connect: adaptive straggler-grace window

Summary

Follow-up to #19407 (broker startup pre-connect for broker-to-server channels). That PR releases startup
early once channels stop arriving for a fixed STRAGGLER_GRACE_MS (2 s) quiet window, so one stuck server
cannot hold the readiness gate for the whole budget. This change makes that window adaptive and bounded:
2 s becomes a floor, the window scales to twice the first observed connect latency, and a new
STRAGGLER_GRACE_CAP_MS (10 s) ceiling keeps it from growing without limit.

• The scaling stops the gap between completion waves — which appears when there are more channels than
  worker threads — from being misread as a straggler and abandoning healthy channels that are only queued.
• The ceiling preserves the straggler protection the window exists for: without it, a very slow healthy
  connect would stretch the window so far that a genuinely dead server would hold the gate for most of the
  budget.

No config, wire, or API changes.

Problem

Pre-connect submits every (server, tableType) connect to a pool capped at MAX_CONNECT_THREADS = 16
and counts completions off an ExecutorCompletionService. Once the first channel is up, each subsequent
poll waits at most the grace window; a quiet window that long is taken as "what's left is stuck", and
startup is released.

A single fixed value cannot satisfy both goals at once:

• Too short and it under-counts when connects are slow. With 48 channels on 16 workers, the surplus
  completes in waves ~one connect-latency apart. If a connect takes ~3 s, the gap between waves exceeds a
  2 s window, so after the first ~16 the next poll times out during the quiet gap and startup releases,
  counting only the first wave (~16/48) even though all 48 were healthy.
• Too long and it defeats the straggler protection: a dead server holds the readiness gate for the
  whole window on every startup.

Fix

The window is max(STRAGGLER_GRACE_MS, min(STRAGGLER_GRACE_CAP_MS, 2 * firstConnectLatency)), extracted as
ServerPreConnector.stragglerGraceMs(...):

• Floor (2 s): unchanged base grace, so a fast cluster releases just as quickly as in Add broker startup pre-connect for broker-to-server channels (SSE) #19407 (2 * L
  stays under the floor when L is small, e.g. a ~120 ms TLS connect).
• Scale (2 * L): since all tasks start together, the first success approximates one connect's latency
  L, and the waves are ~L apart, so a 2L window waits through each wave instead of abandoning it.
• Ceiling (10 s): bounds a dead server's hold to roughly one connect plus the ceiling, regardless of how
  slow the healthy connects are. 10 s (5x the floor) covers realistic healthy connect latencies while
  keeping the dead-server delay well under the budget.

Unchanged: until the first successful connect the whole budget is available (nothing up yet, so "every
server slow" and "a few stuck" are indistinguishable, and a fast failure must not start the clock). The
readiness gate, the pool cap, the per-connect deadline-derived timeout, and the graceful (not
shutdownNow) executor teardown are all untouched.

Known limitation: mixed connect latencies

The window is sized off the first (fastest) connect. When one server is fast but the rest are slower
than the floor, the fast connect pins the window at the 2 s floor, and the slower-but-healthy channels
arrive after it has elapsed — so they are released before being counted (e.g. a cluster with one ~10 ms
server and the rest at ~6 s counts 1). No window value fixes this: a reactive release decision
necessarily fires before the slow channels return, and inflating the window to cover them would impose that
cost on every cluster (including fast ones) — trading the straggler protection back away.

This case is benign: the uncounted channels still finish on their daemon threads (shutdown(), not
shutdownNow()) and are still published for the first query to reuse. Only the returned count and the
startup log under-report; correctness and the lazy-connect fallback are unaffected. It is documented and
tested (mixedLatencyUnderCountsAndIsNotFixedByTheCeiling) so a later change does not "fix" it by inflating
the window for everyone.

What this does not change

• The 16-worker pool cap stays — it is a throughput cap; this change removes the wave under-count without
  touching it.
• No per-connect sub-budget cap is added — that would abort exactly the slow-but-reachable TLS connects
  pre-connect exists to warm.
• Healthy, fast clusters are unaffected: when 2 * L is below the 2 s floor, the floor is used, so the
  window is identical to Add broker startup pre-connect for broker-to-server channels (SSE) #19407.
• Fully within the budget: the window is always min(remaining, ...), so nothing waits past deadlineMs.

Testing

• stragglerGraceScalesBetweenFloorAndCeiling — unit-tests the window math directly across the three
  regimes (floor / linear scale / ceiling), no timing.
• manyHealthyChannelsSlowerThanGraceFloorAllConnect — 48 channels (MAX_CONNECT_THREADS * 3) on the
  16-worker pool, every connect healthy but 3 s (> the 2 s floor). Asserts all 48 connect; a fixed 2 s
  window counts only ~16/48.
• mixedLatencyUnderCountsAndIsNotFixedByTheCeiling — documents the benign mixed-latency under-count
  above.
• oneStuckChannelDoesNotHoldStartupForTheWholeBudget and the other existing grace-window cases still
  hold — a genuine straggler still releases at the (now floor-scaled-or-capped) window, not the whole budget.
• BrokerServerPreConnectIntegrationTest and TlsIntegrationTest are unaffected: everything there connects
  in milliseconds, so 2 * L stays under the floor and no grace window ever expires.

Backward compatibility

No config keys, metrics, wire protocol, or public API change. STRAGGLER_GRACE_CAP_MS is a new internal
constant. Behaviour differs from #19407 only in the many-channels-slower-than-floor case, where it now waits
through the completion waves (still budget-bounded and ceiling-bounded) instead of under-counting.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.70%. Comparing base (c611609) to head (73ab63e).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19519      +/-   ##
============================================
- Coverage     67.74%   67.70%   -0.05%     
- Complexity     1424     1430       +6     
============================================
  Files          3489     3490       +1     
  Lines        224672   224943     +271     
  Branches      35468    35516      +48     
============================================
+ Hits         152210   152292      +82     
- Misses        60445    60616     +171     
- Partials      12017    12035      +18     

Flag	Coverage Δ
integration	100.00% <ø> (+100.00%)	⬆️
integration1	100.00% <ø> (?)
integration2	0.00% <ø> (ø)
java-25	67.70% <100.00%> (-0.05%)	⬇️
lane-a	100.00% <ø> (+100.00%)	⬆️
lane-b	0.00% <ø> (ø)
temurin	67.70% <100.00%> (-0.05%)	⬇️
unittests	67.69% <100.00%> (-0.05%)	⬇️
unittests1	57.80% <ø> (-0.01%)	⬇️
unittests2	39.44% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jineshparakh]

@yashmayya can you please re-review?