yashmayya commented on code in PR #19519:
URL: https://github.com/apache/pinot/pull/19519#discussion_r3971753370
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/ServerPreConnector.java:
##########
@@ -164,6 +171,13 @@ public int preConnect(long deadlineMs) {
break;
}
if (Boolean.TRUE.equals(future.get())) {
+ if (connected == 0) {
+ // All tasks started together, so the first success approximates
one connect's latency. With
+ // more channels than workers the surplus completes in waves one
latency apart, so a window
+ // narrower than that latency would abandon healthy channels
still queued behind the pool.
+ // Scale to twice the first latency, never below the floor.
+ graceMs = Math.max(STRAGGLER_GRACE_MS, 2 *
(System.currentTimeMillis() - startMs));
Review Comment:
**Blocker: the window has a floor but no ceiling, so this re-opens what
#19407 closed.**
#19407 added this window so one dead server cannot hold readiness for the
whole budget (30004 ms -> 2027 ms). `2 * firstLatency` has no cap, so the
slower the cluster, the weaker that protection.
Measured on this branch. 8 channels (7 healthy + 1 black-holed), default 30
s budget:
| healthy connect | master | this PR | opened |
|---|---|---|---|
| 1 s | 3.0 s | 3.0 s | 7/8 |
| 3 s | 5.0 s | 9.0 s | 7/8 |
| 8 s | 10.0 s | 24.0 s | 7/8 |
| 12 s | 14.0 s | 30.0 s (whole budget) | 7/8 |
7/8 in every row. The extra wait opens no channel. At 12 s the early release
is gone.
Sharpest form: take `oneStuckChannelDoesNotHoldStartupForTheWholeBudget` and
change one thing, its healthy connects sleep 5 s instead of returning
instantly. master releases at 7.0 s and passes. This branch releases at 15.1 s
and fails its own 10 s bound. That test passes today only because its connects
are instant.
This shape is 8 channels on a 16-thread pool, so no waves exist. The
description says "every other case is identical to #19407". This case is not.
The new test cannot catch it either: it asserts only `connected == count`,
with no elapsed bound, so it stays green if `graceMs` grows without limit.
Fix: cap the scaling. I tested `min(3 * STRAGGLER_GRACE_MS,
max(STRAGGLER_GRACE_MS, 2 * L))`. It keeps the full win (48/48 on the uniform
case) and caps the 12 s row at 18 s. Pre-connect is on by default, so this
lands on normal deployments.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/ServerPreConnector.java:
##########
@@ -164,6 +171,13 @@ public int preConnect(long deadlineMs) {
break;
}
if (Boolean.TRUE.equals(future.get())) {
+ if (connected == 0) {
Review Comment:
**Blocker: the window keys off the fastest connect, but the gap it must
cover is set by the slowest.**
`connected == 0` is the first success, and `ExecutorCompletionService` makes
that the fastest connect by construction. The review ask on #19407 was a window
above the spread between the fastest and slowest healthy connect. The minimum
is the wrong statistic for that.
48 channels, one connects in 10 ms and the other 47 are healthy at 3 s:
- master: 1/48
- this branch: 1/48
The fix does not engage. It also fails with no waves at all: 8 channels on
the 16-thread pool, one at 10 ms plus 7 healthy at 3 s, gives 1/8 both ways. So
the cause is latency spread, not the pool cap, and the description points at
the wrong mechanism.
The new test passes because all 48 of its connects sleep the same `slowMs`.
That is the one distribution where the minimum equals the mean.
I also tried the obvious repair, re-estimating on every success with
`graceMs = max(graceMs, 2 * elapsed)`. Still 1/48. The release fires before any
slow connect completes, so no reactive scheme can learn the spread in time.
If you keep this shape, scope the claim to uniform latency and add a
mixed-latency test that records the gap.
--
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]
