jineshparakh opened a new pull request, #19519:
URL: https://github.com/apache/pinot/pull/19519
## 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**: 2 s
becomes a *floor*, and the effective window scales to **twice the first
observed connect latency**. That
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.
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 `STRAGGLER_GRACE_MS`; a quiet window that long is taken
as "what's left is stuck",
and startup is released.
The 2 s value is a magic number, and it breaks down in one shape: **more
channels than workers, each
healthy but slower than the floor.** With 48 channels on 16 workers, the
surplus completes in three waves
roughly one connect-latency apart (workers free up and pick up the next
batch). If a connect takes ~3 s
(e.g. a TLS handshake), the gap *between* waves exceeds the 2 s window, so
after the first ~16 connect the
next `poll` times out during the quiet gap, `preConnect` concludes the rest
are stuck, and it releases —
counting only the first wave (~16/48) even though all 48 were healthy and
would have connected.
This is the wave under-count Gonzalo flagged as a follow-up on the original
PR ("it needs to sit above the
spread between the fastest and slowest *healthy* connect").
## Fix
`STRAGGLER_GRACE_MS` (2 s) is now the **floor**, not the window. On the
**first** successful connect —
which, since all tasks start together, approximates a single connect's
latency `L` — the effective window
is set to `max(STRAGGLER_GRACE_MS, 2 * L)`. Because the surplus completes in
waves ~`L` apart, a window of
`2L` is wide enough to wait through each wave instead of abandoning it.
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.
## What this does not change
- The `16`-worker pool cap stays — it is a throughput cap, and this change
removes the cliff's *symptom*
(the under-count) without touching the cap.
- 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 before.
- Fully within the budget: the window is always `min(remaining, ...)`, so
nothing waits past `deadlineMs`.
## Testing
- **New** `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
window counts only ~16/48; the adaptive window (scaled to ~6 s off the
first connect) waits through every
wave.
- Existing `ServerPreConnectorTest` cases still hold, including the
one-stuck-channel early-release test —
a genuine straggler still releases at the (now floor-or-scaled) grace
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. Behaviour only
*widens* the straggler wait
in the many-channels-slower-than-floor case (still budget-bounded); every
other case is identical to
#19407.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
