xiangfu0 commented on PR #19568: URL: https://github.com/apache/pinot/pull/19568#issuecomment-5787849710
@gortiz heads-up: after an offline review we have reverted the auto-detection you asked for, so the thread where I said we had adopted it is now out of date. I would rather flag that here than have you discover it in the diff. Where it landed: `pinot.broker.mse.proto.segment.list` is a plain boolean, default `false`, read from cluster config as well as the static broker config. Cluster config wins and applies to the next query, so an operator turns the encoding on once the rolling upgrade has finished, and off again, without restarting brokers. `ProtoSegmentListPredicate` and the `NEVER` / `SAFE` / `ALWAYS` modes are gone, and so is the per-query `protoSegmentList` option. `QueryDispatcher` holds the flag and is the cluster-config listener, which also removes the `getProtoSegmentListPredicate()` seam on the request handler. The reasoning for preferring an explicit switch over `SAFE`: - The encoding now ships **off**, so a rolling upgrade is safe by construction, in any broker/server order, with no version inference involved. `SAFE` was the mechanism for picking the moment to switch; an operator doing it deliberately picks the same moment with less machinery. - `SAFE` was conservative in ways that would surprise operators: version equality meant a heterogeneous cluster stayed on JSON indefinitely, and an instance config left behind by a decommissioned server kept it off until someone noticed and removed it. - Multi-cluster routing was a genuine hole in the detection, since remote-cluster servers are not watched. It is now simply part of the documented precondition. What that costs, and I want to be explicit about it: the window you were concerned about is still there, in the sense that an operator who turns the flag on too early breaks leaf stages until they turn it back off. Two things that were not true when you raised it now bound the damage: the flag is live, so reverting is one cluster-config call and takes effect on the next query rather than needing a broker restart; and enabling logs a WARN naming the precondition. The failure is also loud rather than silent, per the `ErrorOperator` discussion in the other thread. Everything else from your review is unchanged: the `WorkerMetadata` refactor, decode-accepts-both, unmodifiable custom properties, EXPLAIN no longer carrying segment maps, the setter contract javadoc, the `PinotDispatchPlanner` rewording, and the constructor overloads. The integration tests still compare both encodings end to end; they now flip the flag through cluster config instead of a query option, which also covers the live-reload path. If you still prefer auto-detection, I am happy to reopen that discussion here rather than settle it offline. -- 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]
