gortiz commented on PR #19519: URL: https://github.com/apache/pinot/pull/19519#issuecomment-5618107679
First: I owe you an apology, and it covers both of my earlier reviews on this thread. On #19407 you offered a per-connect sub-budget cap and asked whether I'd prefer that trade. I said "don't add the cap" and handed you a grace-window diff instead — constant, `shutdown()` rationale, test and all. Then on approval I flagged the wave under-count as a follow-up and effectively asked you to *fix* the heuristic. This PR is you doing exactly what I asked, carefully and with good measurements. The problem is that I asked for the wrong thing, and you've paid for it with the floor/ceiling pair, a latency estimator, and a documented "benign under-count" that has to be defended in a test so nobody later tries to fix it. That complexity is mine, not yours. Having sat with it longer, I think the straggler grace window was a mistake in concept, not in tuning, and I'd like to recommend we close this PR and remove the window instead. **Why.** The point of pre-connect is that the broker is not Ready until its channels are warm. A heuristic that releases readiness *before* the deadline, on a guess about what the quiet period means, is working against the feature's own purpose — and the wave measurement I posted on #19407 is what that looks like in practice: a completely healthy cluster marked Ready with 32 of 48 channels still cold, and a WARN blaming a straggler when nothing straggled. Tuning the window narrows the window of misfire; it doesn't remove the category. The deadline is already the safety valve. We don't need a second, cleverer one that fires earlier. **What I'd do instead** — schedule everything, drain the completion service until the deadline, and let the deadline be the only bound: ```diff - long waitMs = connected == 0 ? remainingMs : Math.min(remainingMs, graceMs); - Future<Boolean> future = completionService.poll(waitMs, TimeUnit.MILLISECONDS); + Future<Boolean> future = completionService.poll(remainingMs, TimeUnit.MILLISECONDS); ``` and delete `STRAGGLER_GRACE_MS`, `STRAGGLER_GRACE_CAP_MS`, `stragglerGraceMs`, `graceMs` and `releasedEarly` with it. Everything else in `ServerPreConnector` stays exactly as it is. The single most important property, and the one I'd want pinned by a test: **a connect that fails must not stop us waiting for the others.** One refused or black-holed server should cost us that server, not the rest of the warm-up. That already holds today — `ExecutionException` is logged and the loop continues — and it keeps holding here; I just want it to be the explicit contract rather than a side effect of the loop shape. Three things make the simple version good enough, and I under-weighted all of them when I proposed the window: - **Waves stop mattering on their own.** They were only ever harmful because the heuristic read the gap between them as a stuck server. With `poll(remainingMs)` the surplus is simply slower — 48 channels at 3 s land at 3/6/9 s and all 48 are counted. No estimator, no floor, no ceiling, and the mixed-latency case you documented as an inherent limit disappears too, because it was inherent to the reactive window rather than to the problem. - **A late channel is not a lost channel.** `shutdown()` (not `shutdownNow()`) lets a queued or slow connect finish and publish, so it still warms in the background. - **A query racing an in-flight pre-connect already waits for it.** `ServerChannel.sendRequest` takes `_channelLock` bounded by the query's own timeout, then `connectWithoutLocking()` no-ops on the published channel. So it's per-server, it reuses rather than duplicates, and it needs no new code. Between this and the previous point, releasing early buys much less than I assumed it did. Also worth noting for scope: `preConnect`'s return value is only used for a log line in `BaseBrokerStarter`, so "under-counting" was never the real harm — premature readiness was. **The honest cost, which is the part I got wrong before.** With one unreachable server, every broker holds readiness for the whole budget on every restart. That was exactly my argument for the window on #19407: brokers roll one at a time, so the budget is paid serially down the set — 20 brokers × 30 s ≈ 10 min of extra rollout. I still think that number matters. I just think the answer is a smaller budget rather than a heuristic, because the budget is the same lever with none of the guesswork: the same 20-broker rollout at a 10 s budget adds ~3½ min instead of ~10, and 10 s is still an order of magnitude more than a healthy TLS handshake needs. One predictable number an operator can reason about and tune beats a self-adjusting window with a floor, a ceiling and a documented failure mode. Mechanically this is a small follow-up: close this PR, and a deletion patch against `ServerPreConnector` plus the grace-window tests, optionally with the budget default moved. Happy to write that one myself if you'd rather not spend more time on this — it's my correction to make. Separately and explicitly out of scope: switching the pool to virtual threads would remove the waves entirely rather than merely making them harmless, but it doesn't change the bad case (a black-holed connect runs to the budget however it's scheduled) and it'd be the repo's first virtual-thread usage. Worth its own discussion later, not a reason to hold anything up here. Sorry again for the round trip. The analysis in this PR is genuinely good work — the wave regression test and the mixed-latency case are the clearest description of the behaviour anyone has written, including me — and if we do keep the window for any reason, this is the version I'd want. -- 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]
