timothy-e opened a new pull request, #18947: URL: https://github.com/apache/pinot/pull/18947
This PR is split into multiple logical commits for easier review. The bulk of the logic is in the final commit. ### Summary Adaptive routing violates the same-replica-group invariant required for correctness on upsert tables. This PR restores the latency benefits of adaptive routing while preserving correctness, by making the routing decision at the replica-group level rather than the server level. Instead of the parent class's per-segment independent adaptive routing (which violates the same-replica-group guarantee), this implementation: 1. Groups all query-relevant candidates by pool (replica group) 2. Scores each replica group by its **worst-case server rank** — since scatter-gather query latency is bounded by the slowest server, we want the group whose bottleneck is least bad 3. Picks the group with the lowest worst-case rank 4. Routes **all** segments to that single replica group Replica groups with no stats are preferred. Also adds a flag `pinot.broker.adaptive.server.selector.enable.strict.replica.group` (default true) to control this behaviour, in case we discover any bugs with the implementation - we can switch to a simple "disable adaptive routing for Strict Replica Groups" Fixes https://github.com/apache/pinot/issues/12507 ## Commit breakdown ### 1. Disable adaptive routing for Pinot tables using StrictReplicaGroup Automatically disables adaptive server selection for StrictReplicaGroup tables in `InstanceSelectorFactory`. When the broker has adaptive routing enabled globally (e.g. `pinot.broker.adaptive.server.selector.type = HYBRID`), upsert and dedup tables will now automatically fall back to non-adaptive routing without requiring any table config changes. ### 2. Move ReplicaGroup selector tests to dedicated ReplicaGroupSelectorTest.java Because there's a lot of Adaptive Routing specific logic that will be added to the ReplicaGroupSelector tests, move it to a new file to isolate the behaviour better. Moving this before changing any logic allows the diff in the follow-up commits to be cleaner. ### 3. Replace Pair return type on BaseInstanceSelector::select with a record The protected abstract `BaseInstanceSelector::select` method previously returned `Pair<Map<String, String>, Map<String, String>>`, where the positional meaning of left vs. right was only conveyed by a comment. This replaces that with a self-documenting `record`. `record`s aren't used yet in the code base, so we needed a small change to pom.xml because the linter would incorrectly error with `com.google.googlejavaformat.java.FormatterException: 99:5: error: illegal start of expression`. ### 4. Implement adaptive pool selection for StrictReplicaGroupInstanceSelector The main feature commit (described in Summary above). ## Test Plan On a broker, with adaptive routing enabled and an upsert table, we used the script [`pinot-pool-distribution-grapher.sh`](https://gist.github.com/timothy-e/9768ec0dad525e64d42e4289a5a60fa1) to show: <img width="940" height="332" alt="image" src="https://github.com/user-attachments/assets/6c88c896-be97-4965-9483-2da68583649a"; /> Correctness issues are still unlikely, because we'd need an upsert to span a segment boundary, and then the old and new segments would need to be returned by two different servers. Say: > * Servers p1-1, p2-1 (each different pools) contain the same segments A, B. > * key abc123 was in segment A, but we sealed A so now it's stored in segment B. > * There was an upsert across segment boundaries - so we want to disregard the value for abc123 from A, and use the value of it from B. > * But server p2-1's segment B is slightly out of date and doesn't have the new value of abc123 yet. > > Then, if I ask for segments A, B and I'm willing to go cross replicas, I could get: > > * segment A from p1-1 (won't give me abc123 because it's stale) > * segment B from p2-1 (won't give me abc123 because it doesn't have it yet) > > so we don't return abc123. > > or > * segment A from p2-1 (will give me abc123 because it doesn't know it's stale yet) > * segment B from p1-1 (will give me abc123 because it knows this is the new value) > > so we double-return abc123. **Steady State with this PR**: we hit p1 and p2 roughly equally <img width="3327" height="287" alt="60486140-2e52-41fc-890e-88b97e5591b0" src="https://github.com/user-attachments/assets/df13b78b-f18f-4cad-9660-b92a2786a693"; /> **Steady state with this PR and `pinot.broker.adaptive.server.selector.enable.strict.replica.group` disabled**: we perform the same as above. **When a server in pool p2 is degraded with this PR**: we stop routing immediately to p2 and start routing to p1. When the server is healthy again, we can resume routing to p2. <img width="3339" height="971" alt="d4b30271-6d3a-442b-b3f4-cd641b6e0c3f" src="https://github.com/user-attachments/assets/ddbab7b1-f1c4-4ebc-9ccd-52263bf08d1d"; /> -- 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]
