924060929 commented on PR #65173:
URL: https://github.com/apache/doris/pull/65173#issuecomment-5313561816
Thanks for the work on this framework. The SPI design with
`BackendSelectionProvider` + `BackendSelectionManager` is a clean approach to
making backend selection extensible without modifying kernel APIs. The
validation logic (`validateOrderedCandidates`, `validateCandidatePartition`)
ensuring providers preserve all candidate instances is a good safety contract.
However, I have a few concerns that I think should be addressed before
merging:
---
**1. Cloud mode guard is inconsistent across overloads**
`orderLoadCandidates(ConnectContext, List<Backend>)` checks
`Config.isCloudMode()` and returns early, but the overload
`orderLoadCandidates(SelectionHint, List<Backend>)` does not. Same issue with
`partitionPreferredLoadCandidates` — no cloud mode guard at all.
Currently the call sites happen to guard correctly (e.g.
`LoadBalanceScanWorkerSelector.orderLoadReplicas` checks `Config.isCloudMode()`
before calling in), but this is fragile — the protection relies on every caller
remembering to check. A new call site using the `SelectionHint` overload
directly could silently enable load selection in cloud mode.
**Suggestion:** Add `Config.isCloudMode()` checks at the entry of all public
static methods in `BackendSelectionManager`, not just the `ConnectContext`
overloads. Defense should be in the facade, not delegated to callers.
---
**2. No exception isolation for provider implementations**
All provider method calls in `BackendSelectionManager` only catch
`UserException`. If a downstream provider throws a `RuntimeException` (NPE,
ClassCastException, etc.), it propagates directly to the caller — crashing the
query, load, or repair operation.
Since this framework is explicitly designed for downstream extensions via
`ServiceLoader`, provider quality is uncontrollable. A buggy provider gets
cached as a global singleton in `provider()` and will affect **all**
queries/loads until FE restart.
**Suggestion:** Wrap all provider calls in `try { ... } catch (Exception e)`
at the `BackendSelectionManager` level. On non-`UserException` failures, log a
warning and fall back to default behavior (return the original candidate list).
An SPI framework should isolate the kernel from extension bugs.
---
**3. REQUIRE mode has no graceful degradation**
`requiredCandidates()` throws `UserException` when `preferredCandidates` is
empty. This can happen in realistic scenarios:
- REQUIRE mode selects by `locationTag` (e.g. AZ-a)
- Partition pruning eliminates all partitions whose data resides in AZ-a
- All remaining tablets only have replicas in AZ-b → preferred list is empty
→ query fails
The user setting REQUIRE (possibly via a global session variable) has no
visibility into how partition pruning interacts with the selection constraint.
The error message ("No candidate satisfies required backend selection key
'...'") doesn't explain *why* no candidate was found.
**Suggestion:** At minimum, enrich the error message with diagnostic context
(how many candidates existed, what tags they had, why none matched). Consider
also providing a session variable to allow REQUIRE to degrade to PREFER when no
preferred candidates survive pruning, rather than failing hard.
---
**4. Duplicate selection computation in the query path**
In the query path, selection is computed twice on essentially the same
candidates:
- `OlapScanNode.orderReplicasForQuerySelection()` calls
`orderQueryCandidates(hint, replicas, tagOf)` — ordering `Replica` objects
- `LoadBalanceScanWorkerSelector.partitionPreferredReplicas()` calls
`partitionPreferredQueryCandidates(hint, locations, tagOf)` — partitioning
`TScanRangeLocation` objects
Same hint, same candidates (different representations), two different SPI
methods. This means the provider must implement consistent logic in both
`orderQueryCandidates` and `partitionPreferredQueryCandidates`, or the two
layers may disagree.
More importantly, the `scanBackendOrderBySelection` flag set by OlapScanNode
suppresses shuffle in `PointQueryExecutor`, but the actual worker assignment in
`DistributePlanner` may have overridden the ordering. If these paths ever share
context, the inconsistency could be confusing.
I understand this is partly due to the scan range protocol
(`TScanRangeLocation`) not carrying enough metadata (e.g. `locationTag`,
selection tier) from OlapScanNode to DistributePlanner. Enriching the scan
range protocol to carry selection results from OlapScanNode could eliminate the
duplicate computation and ensure consistency. This could be a follow-up
improvement.
--
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]
