[opt](point query) Optimize backend path collection for point queries - #67151
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: request changes.
I found two actionable issues in the new point-query-only collection path:
- The helper and
Tablet.getQueryableReplicas()can observe different replica snapshots after planning locks are released, allowing a newly published distinct-backend replica to bypass the dead-path tier. - Cloud replicas have no disk path hash, but the helper still performs an additional compute-group/backend placement resolution whose result cannot affect cloud queryability.
Critical checkpoint conclusions:
- Goal and proof: The patch does reduce stable local/remote point-query collection from all cluster backends to selected replica backends, and the regular-scan path remains unchanged. The new unit test correctly proves helper-level narrowing and alive/offline disk-set construction, but it does not cover the accepted concurrency interval or cloud call count.
- Scope and clarity: The source change is focused, but the two issues above need correction before the optimization is safe and complete across supported modes.
- Concurrency and lifecycle: New maps/sets are method-local and introduce no ownership, lock-order, or release problem. The material concurrency issue is the unlocked point-query reevaluation using two independently mutable
LocalTabletsnapshots. - Correctness and error handling: Visible-version checks, replica state/version checks, backend availability, resource tags, and downstream error propagation are otherwise preserved. Stable local/remote identity and missing-backend behavior match the previous path; cloud dynamic identity does not change disk-path correctness because
CloudReplica.pathHashis always-1. - Compatibility and parallel paths: Non-point scans retain the full backend path map. Remote OLAP uses its catalog-scoped backend snapshot. No FE/BE protocol, storage-format, rolling-upgrade, configuration, persistence/EditLog, transaction, data-write, or master-failover change is involved.
- Performance: The main asymptotic improvement is valid for local/remote point queries. The extra cloud resolution is nevertheless provably unused hot-path work; this review does not claim the whole PR is net slower than its baseline.
- Tests and observability: Existing logs/retries remain available, but retry only mitigates the newly admitted dead-path candidate. Add a latch-based snapshot-publication test and a cloud resolution call-count test. No local build or test was run because the review runner instructions prohibit builds.
- User focus: No additional focus point was supplied; the complete PR was reviewed.
Review convergence completed in two rounds. The second full review and both focused rechecks returned NO_NEW_VALUABLE_FINDINGS; no other unresolved issue remains.
TPC-H: Total hot run time: 16628 ms |
TPC-DS: Total hot run time: 81550 ms |
ClickBench: Total hot run time: 14.43 s |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Point-query planning built alive disk path sets for every backend even though tablet pruning leaves only a small set of replica backends. This made planning work grow with cluster size. Build the full map for regular scans and collect paths only for the selected tablet replica backends for point queries. Use the current backend snapshot API so the backport remains compatible with cloud and remote OLAP tables.
Backport of a209ce4b876c38aea0ea7d1f6c84e6e7b14c5a39.
### Release note
Reduce point-query planning overhead in clusters with many backends.
### Check List (For Author)
- Test: Unit Test
- ./run-fe-ut.sh --run org.apache.doris.planner.OlapScanNodeTest#testPointQueryBackendAlivePathsOnlyUseSelectedTabletBackends
- Behavior changed: No. Only point-query planning work is reduced.
- Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Automated review result: request changes.
This head still contains two actionable issues already reported in live inline threads:
- The helper and
Tablet.getQueryableReplicas()can observe different replica snapshots after planning locks are released, allowing a newly published distinct-backend replica to bypass offline-path demotion. - Cloud replicas have no disk path hash, but the helper still performs an additional compute-group/backend placement resolution whose result cannot affect cloud queryability.
No new inline comment is added because both issues are already anchored on the current head; duplicating them would violate the review's deduplication requirement.
Critical checkpoint conclusions:
- Goal and proof: For stable local and remote metadata, the patch reduces point-query disk-path collection from every cluster backend to the selected tablet's replica backends, while regular scans retain full-backend collection. The helper-level unit test proves selected/unrelated/missing backend behavior and alive/offline disk-set construction, but not the two failing production scenarios above.
- Scope and clarity: The refactor is small and focused. It is not ready to approve until the replica snapshot and pathless-cloud cases are corrected.
- Concurrency:
LocalTabletpublishes replica lists with volatile copy-on-write snapshots and lazy point-query evaluation runs without the planning table lock. The new helper and queryability filter take separate snapshots; that is the material race. No new shared map/set, lock-order, deadlock, or heavy-under-lock issue was found. - Lifecycle: The added maps and sets are method-local. No static initialization, ownership cycle, cleanup, or release-order change applies.
- Configuration: No configuration item or dynamic-reload behavior is added or changed.
- Compatibility: No FE/BE protocol, function symbol, storage format, serialized metadata, or rolling-upgrade contract changes. Cloud and remote OLAP subclasses remain in scope; cloud has the redundant placement work described above.
- Parallel paths: Non-point scans preserve the prior full backend path map. Remote OLAP continues to use its catalog-scoped backend snapshot. No other functionally parallel scan path requires the same code change.
- Conditions, correctness, and error handling: Visible-version checks, replica state/version checks, disk-path demotion, backend availability, resource-tag filtering, and downstream failures are otherwise preserved. Backend-map refresh drift predates this diff; the new missing-path case is specifically the two-replica-snapshot race already reported.
- Test coverage: The added JUnit test is deterministic and its expected values are correct by inspection, but it calls the helper directly. Add a latch-based publication test through queryability filtering and a cloud placement-resolution call-count test. No separate third test-derived defect was found.
- Test execution: No local build or test was run because the review-runner instructions prohibit builds. The live check rollup shows FE CheckStyle and the visible formatting, license, secret, and changed-file checks succeeding.
- Observability: Existing logs and retry behavior remain available; retry mitigates but does not remove the offline-path candidate race. No new metric or log is otherwise required.
- Persistence and transactions: No EditLog, replay, master-failover, transaction, or persisted-state change applies.
- Data writes and FE/BE variables: No data-write atomicity, crash recovery, memory ownership, or new FE-to-BE variable propagation concern applies.
- Performance: The intended asymptotic improvement is valid for stable local/remote point queries. The extra cloud placement resolution is provably unused hot-path work. Supported successful point queries enforce a single tablet/partition, so no separate per-partition rescan regression was substantiated.
- Other issues and user focus: No additional valuable issue survived the final sweep. No additional user-provided focus point was supplied; the complete PR was reviewed.
Review convergence completed in one round: two independent full reviews and the separate risk-focused review all returned NO_NEW_VALUABLE_FINDINGS after missed-issue rechecks. Every candidate was independently verified, deduplicated, accepted, or dismissed, and no unresolved point remains.
|
run buildall |
TPC-H: Total hot run time: 16702 ms |
TPC-DS: Total hot run time: 82064 ms |
ClickBench: Total hot run time: 14.65 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
@morrySnow please approve again; resolved conflicts ; |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Point-query planning built alive disk path sets for every backend even though tablet pruning leaves only a small set of replica backends. This made planning work grow with cluster size. Build the full map for regular scans and collect paths only for the selected tablet replica backends for point queries. Use the current backend snapshot API so the backport remains compatible with cloud and remote OLAP tables.
Release note
Reduce point-query planning overhead in clusters with many backends.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)