jineshparakh commented on code in PR #19519:
URL: https://github.com/apache/pinot/pull/19519#discussion_r3975899541
##########
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:
Fixed — the window now has a ceiling. I extracted the math into
`stragglerGraceMs()`:
max(STRAGGLER_GRACE_MS, min(STRAGGLER_GRACE_CAP_MS, 2 * firstLatency))
with `STRAGGLER_GRACE_CAP_MS = 10_000`.
I reproduced your table and swept the cap: a cap C covers healthy connects
up to L ≤ C and bounds a dead server's hold to ≈ firstConnect + C. Your rows
under a 10 s cap: 8 s → 18 s, 12 s → 22 s (vs the 24 s / 30 s uncapped) — both
under the 30 s budget.
I went with 10 s (5× the floor) rather than 3× / 6 s so it also covers
slow-but-healthy connects in the 6–10 s range that a 6 s cap would under-count;
since the dead-server hold stays under budget either way, I spent the extra ~4
s of straggler bound on that coverage. Happy to tighten to 6 s if you'd rather
keep the bound smaller — it's one constant.
On the test gap you flagged: added
`stragglerGraceScalesBetweenFloorAndCeiling`, which asserts the window math
directly across the floor / linear-scale / ceiling regimes, so a `graceMs` that
grows without limit now fails a test. And the description no longer claims
"every other case is identical to #19407".
##########
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:
You're right, and I've scoped it the way you suggested rather than trying to
beat it. Confirmed no reactive scheme fixes it: the release necessarily fires
before the slow connects return, so the window can't learn the spread in time —
I reproduced 1/48 and the no-waves 1/8, and the `max(graceMs, 2 * elapsed)`
repair still gives 1/48.
So, as you asked:
- The description now has a "Known limitation: mixed connect latencies"
section, stating the window keys off the fastest connect and that latency
spread — not the pool cap — is the cause.
- Added `mixedLatencyUnderCountsAndIsNotFixedByTheCeiling` (one fast connect
+ the rest slower than the floor → counts 1), which documents the under-count
so a later change doesn't "fix" it by inflating the window for every cluster.
It stays benign: the uncounted channels still finish on their daemon threads
(graceful `shutdown()`, not `shutdownNow()`) and are published for the first
query, so only the returned count and the log under-report.
--
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]
