xiangfu0 commented on PR #19568: URL: https://github.com/apache/pinot/pull/19568#issuecomment-5768735395
Thanks @gortiz — a very useful review. Everything is addressed in 734bf84 (a new commit on top, so it diffs cleanly against what you reviewed), and each inline thread has a reply. Two of the suggestions I did not take, and I explained why in their threads. **The two main points** - **Auto-detection (adopted):** `NEVER` / `SAFE` / `ALWAYS` with `SAFE` as the default, modeled on `SendStatsPredicate`. The live cluster-config toggle and the runbook are gone. The four deliberate divergences from `SendStatsPredicate` are listed in the thread, including multi-cluster queries always using the legacy encoding, since remote-cluster servers aren't watched. - **Dispatch-loop placement (kept, with reasoning):** the last server starts at the same moment under either ordering, and on master too. Build-first would only delay the other servers. The math is in the thread. **The smaller items** - **`QueryDispatcher` constructor:** the 10-arg signature is back as an overload. It uses `NEVER` rather than the `SAFE` default, since its callers don't watch versions. An unwatched `SAFE` behaves as `NEVER` anyway, and `NEVER` says so explicitly. - **Mutation contract:** documented on both segment-map setters. - **Explain:** segment maps are dropped from EXPLAIN responses rather than re-encoded. I checked that `QueryDispatcher#explain` reads only `getRootNode()` in 1.3.0, 1.4.0, 1.5.0 and master, and the comment now names that reader. - **`PinotDispatchPlanner` comment:** reworded as you suggested. You were right that stage-0 workers never carried segment maps. **Deliberately not changed** - **WARN on a missing segment map:** an old server fails loudly already, via `ErrorOperator` / `UnsupportedOperationException`. The WARN would also only exist in new server code, which is not what runs in that scenario. - **Splitting out the plan-time guard:** squash-merge means a commit split wouldn't survive, and the guard only makes sense together with presence-means-leaf. It is called out in the description instead. -- 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]
