Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68726 Problem Summary: Repeated independent queries for the same remote external file split can choose the same backend under consistent hashing. Each policy starts with zero assigned weight and the original tie rule selects the same candidate, so increasing the existing candidate count does not remove the hotspot. Add an opt-in external_scan_consistent_hash_spread_num session variable. The default value of 1 preserves the existing scheduler. Values greater than 1 select stable, distinct hash candidates and choose the candidate with the least policy-assigned weight, breaking ties uniformly at random. Candidate counts are capped by eligible backends. Local preference and mandatory Host constraints remain in effect; global redistribution is disabled only in the new mode to preserve candidate and locality bounds. Each split executes once. In a local experiment using three backends on one shared physical host, a one-million-row, single-file workload at concurrency 32 improved pooled successful QPS by 2.4234x for COUNT and 2.4143x for SUM across three paired 60-second rounds with file caching disabled. All 12 measurements had zero errors. This does not establish a threefold guarantee or customer result. The round-robin and COUNT changes in the related PR remain separate. ### Release note Add an optional external_scan_consistent_hash_spread_num session setting to spread repeated external scan queries across consistent hash candidates. The default preserves existing scheduling. With file caching enabled, spreading can increase warmup reads and cache space on additional backends. ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - Standard FE build and Checkstyle passed. - FederationBackendPolicySpreadTest: 19 tests passed. - FederationBackendPolicyTest: 9 existing tests passed. - VariableMgrTest#testExternalScanConsistentHashSpreadVariable passed. - Parameter and S3 TVF regression suites generated and verified through the standard regression entry point against the isolated personal cluster. - COUNT and SUM: 100 independent queries each, correct results, one split and one execution target per query, distributed across three backends. - Three paired cache-off capacity rounds for COUNT and SUM passed. - Cold file-cache cost sampling: 30 queries per mode passed; spreading warmed three backend copies and incurred three times the remote reads. - Behavior changed: Yes, only when the new setting is greater than 1 and the scan uses consistent hashing; default scheduling is unchanged. - Does this need documentation: Yes, included in docs/external-scan-consistent-hash-spread.md.
OIiveirra
requested review from
924060929,
CalvinKirs and
morrySnow
as code owners
October 8, 2026 13:18
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68726 Problem Summary: Independently planned queries for one remote split can keep selecting one backend. Enable automatic external scan spreading by default with value 0, using all eligible backends in the current compute group and minimum assigned weight with random ties. Preserve locality and single split execution. Value 1 restores original scheduling; explicit N retains bounded hash candidates. Other backend policy consumers remain unchanged. In a shared-host three-BE cache/hash-off SUM experiment at concurrency 64, three completed alternating pairs increased QPS from 13.867 to 40.506 (2.9211x). The first attempt at pair three had a gRPC keepalive failure in original mode; its one-error 6.10 QPS group is retained separately and the complete pair was repeated after recovery. This is not customer or physical three-host validation. No COUNT, DOP or CPU-routing optimization is added. ### Release note Remote external scans automatically spread across eligible backends by default. Set external_scan_consistent_hash_spread_num to 1 to restore original scheduling. Automatic file-cache scans can increase cache copies and warmup reads. Persisted global settings are retained on upgrade. ### Check List (For Author) - Test: Unit Test / Regression test / Manual test - Standard FE build and Checkstyle; 32 focused FE tests passed. - Two targeted regression suites framework-generated and verified. - 400 correct SUM query profiles verified one split and one target. - Three completed paired C64 SUM measurements; interruption disclosed above. - Behavior changed: Yes, default automatic external scheduling with explicit rollback. - Does this need documentation: Yes, docs/external-scan-consistent-hash-spread.md included.
OIiveirra
changed the title
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68726 Problem Summary: Spread scheduling disables global split redistribution, but still builds and retains alternative Host lists for every split. The only consumer of these lists is redistribution. Automatic mode considers all eligible backends, so these unused lists retain O(split count * backend count) Host references and allocate unnecessary temporary candidate copies. Build alternative Host lists only for the original scheduling mode. Preserve candidate selection, assigned weights, locality and split execution unchanged. Add tests for multiple automatic hash/random splits without retained lists and an original-mode control that still retains its alternative Host. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - 33 focused FE unit tests passed (23 spread policy, 9 original policy, 1 variable). - Standard FE build and Checkstyle passed. - Parameter and external scan regression suites verified on the rebuilt FE. - Behavior changed: No, remove unused allocation without changing scheduling. - Does this need documentation: No, scheduling and parameter semantics are unchanged.
Contributor
TPC-H: Total hot run time: 29040 ms |
Contributor
TPC-DS: Total hot run time: 151725 ms |
Contributor
ClickBench: Total hot run time: 24.38 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68726 Problem Summary: Keep the external scan spreading option at 1 by default so existing scheduling, candidate counts and redistribution are preserved. Explain that 1 is a mode rather than a single-backend limit. Automatic spreading remains available with an explicit 0. Update default-value and strategy tests, explicitly enable automatic mode in the external regression, and remove user documentation from the source repository in favor of the separate website documentation change. ### Release note Preserve original external scan scheduling by default. Enable automatic spreading explicitly with external_scan_consistent_hash_spread_num = 0, or bound hash candidates with a value greater than 1. ### Check List (For Author) - Test: 33 focused FE unit tests; standard clean FE build and Checkstyle; isolated parameter and external scan regressions. Variable output generated by the regression framework. - Behavior changed: Yes; restore the default from 0 to 1 compared with the previous PR revision. Explicit spreading algorithms are unchanged. - Does this need documentation: Yes; English and Chinese website documentation prepared separately, not published yet.
7 of 10 tasks
OIiveirra
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29339 ms |
Contributor
TPC-DS: Total hot run time: 151628 ms |
Contributor
ClickBench: Total hot run time: 24.4 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
previously approved these changes
Oct 9, 2026
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review complete for head 813826a7febf0ea08fd99104017247da106e9673 after three review rounds and a final sweep of all nine changed files. Five distinct P2 issues are inline. There are no existing inline P0/P1 findings to carry forward. No additional user focus was specified. This was static inspection only: I did not build or run the tests; the author's stated test results were not independently validated here.
Critical checkpoints:
- Goal and tests: The default value 1 preserves the prior policy. Explicit spread can distribute remote, hostless splits with randomized minimum-weight ties, as the new policy unit tests exercise. The intended behavior fails for host-hinted remote work (inline M2), pathless bounded-hash work (M5), and a forwarded hash-enabled session (M4). The S3 regression fixture can fail before its scan checks (M3).
- Scope and reuse: The implementation is confined to the FE policy, scan-node policy choice, a session variable, and tests. It reuses the existing hash ring and eligible-backend filtering. No unrelated source edits appear in the nine-file diff.
- Concurrency and locks: Eager assignment occurs during planning; lazy batches call the policy under
SplitAssignment.assignLock, so the query-local weight map is serialized. No new thread or lock order is introduced. Full-ring enumeration under that lock adds avoidable work (M1). - Lifecycle and configuration: The policy captures the session setting when a scan node is constructed; later queries construct new policies. The pre-existing cached hash ring owns no new lifetime here. The nonnegative integer checker and session reset are consistent with the variable framework. The count is forwarded, but its enabling hash selector is omitted (M4).
- Compatibility and parallel paths: No FE–BE Thrift shape or storage format changes. Default scheduling and mixed-mode fallback retain the old policy locally. Eager and lazy file scans share the changed assignment path; metadata and data-generator scans use other placement paths. No cross-language static initialization is added.
- Conditions and correctness: The 0, 1 and positive bounded modes, backend eligibility, required nonremote hosts, and alternative-host use were traced. Each split remains assigned once; no transaction visibility or version invariant is changed. The soft-locality pass can defeat spreading (M2), and placeholder hash identity can confine distinct ranges (M5). The forwarded setting mismatch is a silent behavior change (M4).
- Coverage and results: The new Java policy and variable tests and both regression suites and output files were inspected. Their recorded rows and variable outputs match the test SQL, but they do not cover the forwarded flag pair or host-hinted/pathless multi-backend cases. The OUTFILE path-style key is wrong on both exports (M3). No tests were executed in this review.
- Observability, writes and performance: Existing assigned-weight profile data remains available; no additional metric requirement was established. This changes scheduling, not transactions, data writes or EditLog behavior. The material performance risks are repeated full-ring work (M1) and unused eligible BEs (M2/M5). No other substantiated issue remained after deduplication and the final sweep.
### What problem does this PR solve? Problem Summary: External scan scheduling can hash pathless connector ranges to the same key, and repeated locality hints can keep remote splits on one backend. Add connector-provided split identities and balance spread candidates by assigned weight while preserving the default scheduling behavior. ### Release note None ### Check List (For Author) - Test: Connector SPI surface and metadata tests; focused FE UT attempted but blocked by the read-only Maven repository. - Behavior changed: Yes (opt-in external scan spread scheduling). - Does this need documentation: No
### What problem does this PR solve? Problem Summary: Full-ring construction is unnecessary when spread mode already allows every eligible backend, and the external scan regression fixture used an unrecognized path-style property. Use the filtered backend list directly and align both OUTFILE exports with the recognized property. ### Release note None ### Check List (For Author) - Test: git diff --check; regression fixture review. - Behavior changed: No. - Does this need documentation: No
Contributor
TPC-H: Total hot run time: 29102 ms |
Merged
3 of 16 tasks
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28696 ms |
Contributor
TPC-DS: Total hot run time: 151645 ms |
Contributor
ClickBench: Total hot run time: 24.65 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
rerun p0 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Complete static review of all 27 changed files at 22d6ebafe16dfdfaa76e25357a106028deab5037, after two rounds. Each round included the main risk scan/validation, normal full-scope aspect reviews, and a separate focused risk scan. All three fresh round-2 reviewers returned NO_NEW_VALUABLE_FINDINGS against the unchanged final comment set; the final changed-file and unresolved-candidate sweep is complete.
Three new P2 findings are inline: M1, MaxCompute read sessions share session-relative scheduling coordinates; M2, Iceberg JNI metadata tasks share a dummy-path identity; M3, singleton streamed batches redundantly rebuild the maintained weight histogram. These concern scan parallelism and FE planning overhead; no wrong-result or crash consequence was substantiated. They are distinct connector instances/triggers from the existing threads.
The earlier P1 connector API-major issue and legacy ring-order issue are fixed at this head, so no existing P0/P1 blocking IDs apply. The earlier forwarding, Fluss lake-tail identity, S3 fixture key, all-candidate ring walk, and overloaded-host-hint concerns are addressed in the code. The original per-hinted-split minimum scan was replaced; M3 covers a distinct rebuild cost introduced by that repair. The existing Remote Doris same-URI/different-ticket P2 still applies and is not reposted.
The reviewed execution chain is:
session settings / forwarded map -> ExternalScanNode(policy snapshot)
-> eligible compute-group BEs -> eager planner or lazy SplitAssignment(assignLock)
-> locality / candidate selection -> minimum weight / ties -> scan ranges -> BE
ConnectorScanRange -> PluginDrivenSplit(captured identity,start,length) -> SplitHash
Applicable critical-checkpoint conclusions:
- Goal and evidence: Opt-in 0/all or N/bounded spreading selects minimum assigned weight with uniform random ties while default1 preserves legacy scheduling. Scheduler unit and S3 regression cases support the intended modes statically; MaxCompute and Iceberg metadata work identities remain incomplete (M1/M2), and the existing Remote Doris gap remains. No test execution is claimed.
- Scope and abstraction: Generic scheduler/session changes and connector-owned identity hooks keep planning connector-agnostic. Two optimizer-test pattern edits retain their aggregate assertions and affect no production rewrite.
- Concurrency and lock cost: Eager scheduling is planning-thread-owned. Async partition and streaming producers serialize mutable weights/counts through SplitAssignment.assignLock; range conversion and queue IO run outside it. No new lock order/deadlock found. M3 flags redundant histogram reconstruction inside this lock.
- Lifecycle, initialization and memory: Each scan owns its policy, cumulative weights and captured immutable identities; cached rings are read-only and keyed by eligible backend membership. No new resource, cleanup cycle, thread or C++ static initializer. Java temporary histogram allocations are the M3 concern; BE memory ownership is unchanged.
- Dynamic configuration: Added session/global setting defaults to1, accepts0 through Integer.MAX_VALUE, rejects negative/overflow/nonnumeric statement values before mutation, and is read when each scan policy is constructed. SET/UNSET and global/new-session behavior use existing machinery without restart.
- Compatibility and rolling upgrades: Connector API major14 is propagated to kernel resource, plugin jar manifest and pinned surface test; incompatible plugin majors are rejected. Default1 now retains the original ring order. Renamed engine hook has no stale callers. No new wire/storage layout; older masters cannot implement this new option, while unknown forwarded keys are ignored.
- Thrift/protobuf definitions: No gensrc/thrift or gensrc/proto change; cross-branch field-ID comparison is not applicable.
- Parallel execution paths: Eager, singleton streaming, partition-batch, hash/random/round-robin, native/JNI/system-table and cloud/shared-nothing compute-group paths reviewed. M1/M2 cover missing identities in two previously unhandled arms; Remote Doris is already covered by an existing thread.
- Conditions and error behavior: Mode gates match0/1/N semantics; unavailable required hosts/backends fail; candidate count is capped. Explicit local-preference exception may select outside hash candidates and remain deterministic for a first hinted split; overloaded later hints enter spread. No new silent-error continuation found.
- Test coverage: Unit cases cover weights across batches, random ties, membership/availability, candidate caps, multiple BE counts, forwarding, defaults and invalid values; connector tests cover distinct work and stable wrapped files. Regression cases cover SQL results and settings. Missing MaxCompute-session and Iceberg-metadata identity cases are part of M1/M2.
- Changed expected results: Both complete goldens match inspected ordered SQL, five remote-mode repetitions, duplicate rows from two files, SHOW variable values and unchanged values after invalid SET. Generation provenance and runtime success remain unverified; no files were regenerated or tests run.
- Observability: Existing assignment-weight SummaryProfile, planning timestamps, backend/range EXPLAIN details and debug logs provide scheduling evidence. No new external IO/distributed protocol requiring additional operational counters is introduced.
- Transactions and persistence: Global session defaults use existing VariableMgr EditLog/checkpoint/replay machinery. No transaction lifecycle, committed-data visibility, visible-version or delete-bitmap code changed.
- Data writes and atomicity: Production write/commit paths are unchanged; regression fixture inserts/OUTFILE follow existing patterns. No new write atomicity or crash-cleanup mechanism is introduced.
- FE-to-BE variables: The new option is consumed by FE planning and forwarded between FEs together with the selector/cache flag; BE receives existing assigned scan ranges. No new BE-bound field or scattered transmission path needs updating.
- Performance: Full-candidate spread avoids hash-ring traversal and unused alternative-host lists; minimum ties use one-pass reservoir sampling. M3 identifies avoidable all-backend histogram work on each streamed range; M1/M2 identify avoidable candidate collapse. No local throughput measurement or validation of author benchmark claims was performed.
- Other issues and user focus: No additional user focus was supplied. All initial risks and candidate duplicates have concrete dispositions; no extra wrong-result, availability or lifecycle issue was substantiated after full-scope rechecks.
Review was static only. No builds, tests, source changes, golden regeneration or benchmark runs were performed. Author-reported validation is not treated as executed evidence from this review. No additional user focus was supplied. Review analysis is complete; the three inline P2 findings and existing Remote Doris concern remain for the author to address.
…ghts ### What problem does this PR solve? Related PR: apache#68791 Problem Summary: MaxCompute ranges from different read sessions and Iceberg JNI metadata tasks can share synthetic paths and coordinates. Bounded consistent-hash scheduling then confines distinct work to the same backend candidates. Include the existing read-session or serialized-task identity through the connector hook while preserving native Iceberg file identities. Remove the redundant per-batch weight histogram rebuild because production weight updates already maintain it incrementally. Cover identity separation, ten-backend bounded candidate assignment and histogram consistency across singleton batches. ### Release note Synthetic MaxCompute and Iceberg JNI metadata scan ranges use work-specific consistent-hash identities. This can change their backend placement, including legacy hash mode. Native Iceberg file identities are unchanged. ### Check List (For Author) - Test: Unit Test / Manual test - 70 targeted FE and connector unit tests passed; standard build.sh --fe and Checkstyle passed; git diff --check passed. - Behavior changed: Yes, distinguish synthetic work for hash placement and remove redundant histogram rebuilds. - Does this need documentation: No, existing session-variable semantics are unchanged.
### What problem does this PR solve? Related PR: apache#68791 Problem Summary: Remote Doris result streams can advertise the same proxy URI with different Flight tickets. Hashing only that URI restricts distinct streams to the same bounded backend candidate set. Capture an immutable identity from the URI and the remaining ticket bytes when the split is created, without consuming the execution ByteBuffer. Add tests for distinct work, stable identity, heap/direct/read-only buffer state, and bounded assignment across eight backends. ### Release note Remote Doris consistent-hash placement distinguishes different result-stream tickets at the same endpoint. ### Check List (For Author) - Test: Unit Test / Manual test - 82 targeted FE/connector unit tests passed; standard clean FE build with Checkstyle passed; git diff --check passed. - Behavior changed: Yes, hash placement includes the result-stream ticket; execution parameters are unchanged. - Does this need documentation: No, existing session-variable semantics are unchanged.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28750 ms |
Contributor
TPC-DS: Total hot run time: 151550 ms |
Contributor
ClickBench: Total hot run time: 24.66 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#68791 Problem Summary: Merge the current master while preserving its initialized backend order and narrowed candidate membership alongside incremental split weights. Adapt deterministic selection tests to each policy's randomized order. Repair the attached FE CI failures: assert the complete aggregation keys that remain under outer-join null extension; explicitly request JSON in HTTP forwarding tests; let the test server itself allocate an ephemeral port and shut it down after the suite; exclude embedded multi-release vendor class variants only from Paimon S3 coverage reporting. Production HTTP forwarding, optimizer rules and runtime plugin contents are unchanged. The separate FE coverage job was blocked by the same scheduling merge conflict. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - 76 targeted FE unit tests passed with zero failures, errors or skips. - JaCoCo report reproduction failed without exclusions and passed with the coverage-profile exclusions on the same classes and agent-generated exec file. - Standard FE build and Checkstyle passed; git diff --check passed. - Behavior changed: No additional query or runtime plugin behavior change; preserve both scheduling changes during the master merge and repair test/report infrastructure. - Does this need documentation: No, session-variable semantics are unchanged.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28686 ms |
Contributor
TPC-DS: Total hot run time: 152408 ms |
Contributor
ClickBench: Total hot run time: 24.81 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
924060929
approved these changes
Oct 14, 2026
Collaborator
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed commit 74c3f7664fc8714f167f334e59c023632afb0db4 across all 35 changed files. Review converged after 1 round: both full-scope reviewers and the additional risk reviewer returned NO_NEW_VALUABLE_FINDINGS; every candidate/risk was independently checked and dismissed or deduplicated. No new inline findings and no still-applicable existing P0/P1 findings.
Both earlier P1 concerns are fixed: connector API gating now uses major 14 with the updated surface baseline, and disabled-spread cache affinity retains hash-ring order.
The existing P2 all-candidate ring-walk concern remains partially applicable to TVFs and is recorded as already covered. FileQueryScanNode initializes the TVF policy twice, and the list overload appends backend candidates both times. With 10 distinct eligible BEs, the list has 20 entries; spread=11 therefore asks the ring for 11 unique nodes although only 10 exist, traversing all virtual nodes per split. Automatic spread=0 bypasses this. This is redundant planning work; no wrong-result consequence was established. It is not reposted inline.
Critical checkpoint conclusions:
- Goal and proof: the code provides opt-in minimum-weight placement with random minimum-weight ties, legacy mode 1, automatic mode 0, and bounded hash candidates above 1. Local host preference remains an explicit exception: independent hinted queries may still prefer the same local BE. Policy, identity and variable tests plus S3 regression suites cover the behavior by inspection.
- Scope and clarity: scheduling and identity logic remain centralized and connector-agnostic. The additional HTTP fixture, optimizer assertions and coverage configuration broaden the diff; their mechanisms were reviewed independently.
- Concurrency and locking: eager assignment runs during planning; concurrent partition workers and streamed assignments serialize policy weight/histogram updates under SplitAssignment.assignLock. Connector planning, queue conversion and backpressure remain outside it. No new lock-order cycle was found; the existing TVF ring work remains the performance exception above.
- Lifecycle and initialization: strategy/spread are captured per scan node; weight history persists across batches. Histogram writes are centralized and incremental. Remote Doris snapshots an immutable identity from a duplicate ticket buffer without consuming the execution buffer. HTTP test teardown closes the server context. No C++ static-initialization change.
- Dynamic configuration: the nonnegative int variable defaults to 1, validates negatives/non-integers/overflow, and affects subsequent plan construction without restart. Spread, hash selector and cache settings forward to the master; UNSET restores the compiled default. Persisted global values use existing variable machinery and are not reset by the new default.
- Compatibility: connector API 14 is stamped and checked from the same parent property; incompatible plugin majors are rejected and require matching builds. The new hook is in the frozen surface. No storage format or FE/BE wire change was found; missing session JSON keys retain defaults.
- Thrift/protobuf IDs: no gensrc definition changes; not applicable.
- Parallel paths: eager, partition-batch and streamed planning use the common policy. Cache/hash and automatic RANDOM gates, cloud/group eligibility, mandatory nonremote hosts, native/JNI connector arms, and unchanged Hive/Hudi/ES/JDBC peers were traced. Load policies retain their separate round-robin path.
- Conditions and errors: spread-mode checks distinguish legacy/automatic/bounded modes; overloaded remote hints fall back to spread candidates while mandatory hosts remain exact. Empty eligible/required-host sets fail; assignment arithmetic checks overflow.
- Test coverage: unit additions cover multiple BE counts, candidate reachability, ties, batch history, eligibility, missing hosts, invalid variables, forwarding, identity distinction and buffer preservation. Regression suites check results across modes. Policy-only tests do not exercise the TVF double initialization, and these suites do not measure live throughput.
- Changed results: ordered three-row/NULL fixtures, the six-row two-export result, and variable default/change/error/unset goldens match the SQL by inspection. Golden generation was not reproduced.
- Observability: existing scan backend IDs, planning debug timings and optional assigned-weight profiles remain available. No shared live-load metric is introduced or promised.
- Transactions and persistence: no visible-version, delete-bitmap, transaction or EditLog schema changes. Global variable save/replay follows existing generic handling; no new master-failover transaction path.
- Writes and crash atomicity: production data-write semantics are unchanged; OUTFILE is regression fixture setup. No new storage ownership or crash-atomicity mechanism.
- FE/BE variable passing: spread and selector control FE placement and need FE forwarding only; existing cache options and execution range payloads remain unchanged.
- Performance: spread omits unused alternative-host lists and global redistribution; all-candidate direct lists avoid ring walks on unique inventories. Minimum lookup uses incremental counts instead of per-batch/per-hint full scans. Remaining TVF work is the existing P2 above; no benchmark was executed here.
- Further review and core invariants: the review continued through all changes, producer/consumer chains and missed-case sweeps. No new substantiated visibility, memory-ownership, silent-error or correctness finding remained.
No additional user-provided focus was supplied. This was a static review only: no builds, unit/regression tests, golden regeneration or performance experiments were run, and author/CI claims are not executed evidence from this invocation. No unresolved static coverage gap remains; external plugins were assessed through the supplied scan-range contract, not every third-party implementation.
Review converged after 1 round(s).
hello-stephen
pushed a commit
that referenced
this pull request
Oct 14, 2026
… at graceful exit (#68942) Problem Summary: **In short.** The NonConcurrent regression pipeline fails a few percent of its runs with `Lsan fail` while every case passes. LeakSanitizer finds, at the BE's graceful exit, one stream load's `ResourceContext` that was never released, but its report only shows allocation stacks, which are the same for every stream load, so nobody can tell which load it was. This PR makes the BE print, at graceful exit in memleak-check mode, every `ResourceContext` still alive with its task id, the references still held, its memory tracker and workload group, and whether its `QueryContext` is already gone. Nothing changes unless `enable_graceful_exit_check=true`. **Background.** - With `enable_graceful_exit_check=true` (memleak-check mode, used by the regression pipelines on the ASAN build), `doris_main` stops every service and runs `ExecEnv::destroy()` instead of `_exit()`, so that LeakSanitizer checks the heap when the process exits. The NonConcurrent pipeline greps `be.out` for `detected memory leaks` after `stop_be.sh --grace` and reports `Lsan fail`. - Every query or load has a `ResourceContext`: CPU/memory/IO contexts, a task controller, the query memory tracker and the workload group. The `QueryContext` owns it, and the components that work for the task (load channel, memtables, scanner context, `RuntimeQueryStatisticsMgr`, threads attached to the task, ...) hold references of their own while they run. **The problem, and what it cost.** - `Lsan fail` hits 1–5% of NonConcurrent runs depending on the month (448 of the last 14000, back to March 2026), on PRs that have nothing to do with it; authors rerun or skip. Every report TeamCity still keeps (10 runs from 2026-10-05 to 2026-10-12, on #68707, #68281, #68791, #68803, #68798, #68543, #68901, #68919, #68879) has the same shape, down to the byte counts, and all of its leaks are indirect: | Leaked | Size | Origin | |---|---|---| | one `ResourceContext` with its CPU/memory/IO contexts, `QueryTaskController` and both trackers (`Load#Id=…`, `Memtable#Load#Id=…`) | 6.5 KB, 94 objects | an HTTP stream load: `StreamLoadAction::_process_put` → `StreamLoadExecutor` → `FragmentMgr::_get_or_create_query_ctx` | | the `normal` workload group created by `WorkloadGroupMgr::create_internal_wg()` | 6.1 KB + 4.3 KB | kept alive by `ResourceContext::_workload_group` | | the `std::make_shared` block of the load's `QueryContext` | 2.5 KB | kept allocated by the `weak_ptr` in `QueryTaskController` | - The `QueryContext` itself was destroyed: in `be.INFO`, each of the 21894 `query_id: … use wg:` lines of the run has its `Query … deconstructed`. So one reference to the load's `ResourceContext` is never released, from a place LeakSanitizer does not scan (a bthread stack, whose memory brpc maps itself) or a count that is never decremented. LeakSanitizer cannot tell which of these two it is. - A NonConcurrent run issues about 430 HTTP stream loads (about 380 through the load channel, 150 of them audit-log loads), and the report cannot say which one leaked: the stacks are identical, and the one thing that would identify the load, the tracker label with its id, is inside the leaked memory, which LeakSanitizer does not print. Without the load there is nothing to follow, so the flake stays unexplained. **How this PR fixes it.** - In memleak-check mode each `ResourceContext` registers itself in a process-wide set when it is constructed and removes itself when it is destroyed. The set stores the addresses bitwise inverted, so LeakSanitizer does not take it for a reference and still reports a leaked context. - `ResourceContext::describe_live_contexts()` returns one line per context still alive: address, strong references still held, task id, finished flag, running time, memory tracker (label, consumption, peak), workload group and, for a query, whether its `QueryContext` is still alive. - `doris_main` calls `ResourceContext::log_live_contexts()` right after `ExecEnv::destroy()` on the graceful exit path. It writes the description to `be.WARNING` and to `be.out`, where the LeakSanitizer report follows once `main()` returns, or one INFO line when no context is alive. The printed address falls inside one of the `Objects leaked above` ranges of that report. - The description is built on a short-lived thread, and the main thread only handles the resulting text. Describing a context touches it; done on the main thread, it could leave a pointer to a leaked context in a dead slot of the main thread's stack, which LeakSanitizer scans at exit — it would then take the context for referenced and not report it, so the run that reproduces the leak could pass. The stack of a finished thread is not scanned. **Results.** What the next `Lsan fail` will tell, compared with today: | | Before | After | |---|---|---| | Which query or load leaked | unknown | `task_id=…`; its label, table and test are then in `be.INFO` | | How many references are left | unknown | `references=N` | | Whether the `QueryContext` is gone | only by pairing ~20k log lines | `query_context=destroyed` | | Where to look | the LeakSanitizer report in `be.out` | the same `be.out`, just before that report (the build log's `head -n 100 be.out` shows it) | | Cost with `enable_graceful_exit_check=false` (default) | — | one boolean check per `ResourceContext` construction and destruction | A line looks like this (values made up): ``` 1 resource contexts are still alive. LeakSanitizer reports a leaked context as the memory block that holds it, so match the addresses below against the ranges it prints under 'Objects leaked above'. 0x7c0000001010: references=1, task_id=1eac0c4ec4ec0000-0000000000000003, finished=true, running_ms=7569, mem_tracker=Load#Id=1eac0c4ec4ec0000-0000000000000003 (consumption=0, peak=17.18 MB), workload_group=normal(1), query_context=destroyed ``` **Classes, and how they call each other.** - `ResourceContext` (`resource_context.{h,cpp}`): registers in the constructor and unregisters in the destructor when memleak-check mode is on; `describe_live_contexts()` formats the live ones; `log_live_contexts()` runs that on its own thread and logs the result. - `QueryTaskController`: new `is_query_context_alive()`. - `doris_main`: one call to `log_live_contexts()` after `ExecEnv::destroy()`. ``` doris_main (graceful exit, enable_graceful_exit_check=true) ├─ ExecEnv::destroy() └─ ResourceContext::log_live_contexts() ──► be.WARNING + be.out (or one INFO line) └─ [short-lived thread] describe_live_contexts(100) ├─ registry of disguised addresses ◄── ResourceContext() / ~ResourceContext() └─ per live context: ├─ TaskController: task id, finished, running time ├─ MemoryContext → MemTrackerLimiter: label, consumption, peak ├─ WorkloadGroup: name, id └─ QueryTaskController::is_query_context_alive() ``` No holder of `ResourceContext` changes, and neither do the LeakSanitizer options or suppressions. Once the leaked load is known, the fix for the leak itself will be a separate PR. --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Related PR: #68726
Problem Summary:
Repeated independently planned queries for a single remotely accessible external file split can select the same backend under consistent hashing. The candidate order is stable, each query starts with zero assigned weight, and the existing minimum-weight tie rule repeatedly selects the same candidate. Increasing the configured candidate count alone does not remove this hotspot.
Add the session variable
external_scan_consistent_hash_spread_numwith default 1, preserving existing scheduling:1(default)0N > 1In spread mode, select the candidate with the least query-local assigned split weight and break ties uniformly at random. Assign each split once, retain weight history across batches, and honor compute-group eligibility, local preference and mandatory Host constraints. Candidate counts are capped by the eligible BE count; the implementation is not specific to three BEs.
Disable global split redistribution only in spread mode so it cannot move a split outside its candidate or locality constraints. Do not allocate unused alternative-host lists in this mode. With many splits, balancing is limited to each split's candidates and can be less even than the original global redistribution.
This PR adds no live CPU monitoring, shared query counter, COUNT pushdown, DOP adjustment or BE cache protocol. It is independent of #68726 and does not include that PR's commits. The existing round-robin path remains available; when #68726 is merged, its round-robin fix also remains in effect for original-mode callers.
The variable does not enable file caching. With caching enabled, automatic mode can warm copies on every eligible BE and increase remote warmup reads and cache storage. Persisted global values survive upgrades; restoring the compiled default to 1 does not reset an already saved global 0.
UNSET VARIABLErestores the compiled session default 1.Manual performance evidence, collected on scheduling revision
7e4fb1c549dbefore this final default-value adjustment: the same independent FE binary, three unchanged BEs on one shared physical host with distinct logical Hosts and disjoint nonexclusive four-core affinities; one million-row single-file CSV SUM; file cache and consistent hashing enabled; all candidate caches warmed; concurrency 64; 10-second warmup and 60-second measurement window; three alternating pairs changing onlyspread=1versusspread=0.1)0)The pooled ratio was 2.9324x, with paired ratios 2.8782x–2.9603x. All six groups had zero query errors. QPS counts correct completions within the fixed window; late completions are excluded, all workers are joined, and latency includes the tail of queries started within the window. The original hot BE used 97.5%–98.7% of its four-core capacity; automatic mode used 87.2%–97.7% across the three BEs. Whole-invocation remote and peer read increments were zero.
This evidence applies to the measured warm-cache shared-host workload. It is not a new benchmark of final revision
813826a7feb, a cold-start result, a strict >=3x guarantee, a three-physical-host acceptance result or customer validation. The final adjustment changes the default and documentation, not the explicit spread-selection algorithm; spreading must now be explicitly enabled to seek this benefit. No throughput from the related COUNT optimization is attributed to this PR.Release note
Add opt-in spreading for remote external file scan splits.
external_scan_consistent_hash_spread_numdefaults to 1 and preserves original scheduling. Set it to 0 to spread across all eligible BEs, or a value above 1 to bound hash candidates when file cache or consistent hashing is enabled. Spreading can increase cache copies and warmup costs. Persisted global values are retained on upgrade.Check List (For Author)
Test
Validation performed on the final revision:
run-fe-ut.sh --run 'FederationBackendPolicySpreadTest,VariableMgrTest#testExternalScanConsistentHashSpreadVariable,FederationBackendPolicyTest': 33 tests passed, zero failed/error/skipped. Coverage includes 1/2/3/5 BEs, random ties, multiple batches, locality, bounds, forwarding, original-mode compatibility, default 1 and explicit 0 strategy selection.build.sh --fe --clean -j16and integrated Checkstyle passed. An earlier production build failed with incomplete class/type errors; its evidence was retained and the clean build succeeded without unrelated source edits.run-regression-test.shwith an isolated personal configuration:-g p0 -d query_p0/ddl/set_and_unset_commands -s test_external_scan_consistent_hash_spread_variable; output generated by the framework using-forceGenOut, then verified normally.-g external -d external_table_p0/tvf -s test_external_scan_consistent_hash_spread; explicitly enables 0 for automatic-path coverage.7e4fb1c549d, described above. Each mode was also certified by 100 query-owned profiles: correct result, one source split/range, one positive execution target, one million rows read, local-cache reads and no remote reads. Original mode selected one BE; automatic mode selected all three.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)