Repository navigation
branch-4.2: [improvement](datasource) Bound and fairly schedule external scan planning (#69298) - #69986
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
1 similar comment
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
FE UT Coverage ReportIncrement line coverage |
There was a problem hiding this comment.
Converged after 2 static review rounds against PR head 4b882f7. The review covered all 33 changed files, the fair executor and split-fetch lifecycle, Hive/Hudi/FileSplitter batch paths, and Iceberg manifest/cache streaming and partition evolution. Two verified P1 findings are included inline: Hudi can miss cancellation before a blocking filesystem-view sync, and Iceberg batch manifest streaming cannot adapt historical DATE partitions after DATE-to-TIMESTAMP_NANO promotion. The executor wakeup/close paths and normal Hive split/error/empty/cold-cache behavior were traced and no additional issue was substantiated; Hudi retains its eager assignment path and split-count estimates are advisory only. There was no additional user-provided review focus and no existing blocking inline finding to carry forward. This was a static-only review under the supplied contract; builds and tests were inspected but not executed.
Review converged after 2 round(s).
| // Only provide the unique ID of split source to backend. | ||
| splitAssignment = new SplitAssignment(backendPolicy, this, this::splitToScanRange, | ||
| locationProperties, pathPartitionKeys, admissionResult); | ||
| getScanTaskLease(); |
There was a problem hiding this comment.
[P1] Hudi can miss cancellation before syncFsView starts. Registering the statement lease here lets cleanup stop the SplitAssignment while Hudi's BatchFsViewOwner has not yet published its sync thread. SplitAssignment.addCloseable then closes the owner with syncThread still null; Hudi subsequently calls beginSync and the potentially blocking syncFsView unconditionally (lines 738-744), so the stop interrupt is missed and a cancelled query can remain blocked on remote timeline I/O. Check the stop state before entering sync or make owner publication/close atomically interrupt an already-stopping sync.
There was a problem hiding this comment.
Fixed in 0068bcf. Both Hudi fs-view owners publish the sync thread before checking cancellation, so an already-closed owner rejects the remote sync; a later close interrupts the published thread. Batch beginSync is inside the terminal catch/finally to preserve exact lease release. Added deterministic close-before-publication cases alongside the existing blocked-sync tests; all 339 related FE tests and full FE packaging/Checkstyle pass.
| } | ||
| return decimal; | ||
| } | ||
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); |
There was a problem hiding this comment.
[P1] Preserve historical DATE partition values across DATE-to-TIMESTAMP_NANO evolution. DorisDataTableScan rebinds historical manifest specs to the current schema, so ManifestCacheValue.forScan reaches this unsupported conversion for old date-partitioned files and throws. In batch mode this runs lazily from IcebergScanNode's manifest iterator after the manifest-cache fallback boundary, causing the query to fail instead of scanning the historical partition (and dropped promoted fields can also mismatch equality-delete partition keys). Add a correct DATE/TIMESTAMP_NANO adaptation or retain the historical spec/value for matching, with a partitioned streaming regression test.
There was a problem hiding this comment.
Fixed in 0068bcf. Query-local adaptation now converts DATE/nanos exactly and normalizes dropped temporal partition keys without overflow. Delete manifest caching also preserves the manifest's physical partition types, matching the data-file reader, so current schemas cannot erase or reinterpret historical partition values before adaptation. Added partitioned streaming tests using the production cache readers: old DATE data retains its equality delete both before and after the promoted identity field is removed. All 339 related FE tests and full FE packaging/Checkstyle pass.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static exact-head review of PR 69986 at 0068bcf. The new scheduler, bounded split queues, Hive/Hudi streaming producers, lazy FileSplitter, session/config forwarding, Iceberg manifest prefetch, historical partition adaptation, delete indexing, cancellation, and generation cleanup were reviewed across the complete changed-file list. Three Iceberg issues are verified below. The existing DATE/TIMESTAMP_NANO Iceberg thread and Hudi close-before-sync thread were revalidated as fixed at this head and are not reposted. No additional user review focus was provided. No builds or tests were executed; this was a static-only review. The review converged after 2 rounds.
Review converged after 2 round(s).
| } | ||
| return decimal; | ||
| } | ||
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); |
There was a problem hiding this comment.
[P1] Support DATE to ordinary TIMESTAMP partition promotion. The adapter converts DATE only when the target is TIMESTAMP_NANO; an old identity-partitioned DATE file rebound to an ordinary Iceberg TIMESTAMP (microseconds) reaches the final unsupported-conversion exception. DorisDataTableScan rebinds historical specs by field ID, and the batch manifest iterator evaluates this adaptation lazily, so the producer fails the query instead of reading the historical partition. Add an exact days-to-micros conversion with overflow checks and a partitioned DATE-to-TIMESTAMP regression covering data and delete files.
There was a problem hiding this comment.
Fixed in 68ef8c2. DATE partitions now convert exactly to ordinary without-zone TIMESTAMP microseconds, with overflow checks and exact-midnight checks for reverse historical adaptation. DATE-to-zoned timestamp conversions are rejected for both precisions. Added real partitioned streaming metadata cases and unit tests for positive/negative days, nulls, overflow and lossy narrowing. This is a reader repair; SQL ALTER type support is unchanged. All 253 related FE tests and full FE packaging/Checkstyle pass.
| if (source.typeId() == Type.TypeID.FLOAT) { | ||
| return ((Float) value).doubleValue(); | ||
| } | ||
| if (source.typeId() == Type.TypeID.DATE) { |
There was a problem hiding this comment.
[P1] Normalize dropped DATE and ordinary TIMESTAMP keys consistently. In the UNKNOWN target branch, dropped DATE values become BigInteger days multiplied by NANOS_PER_DAY, while ordinary TIMESTAMP values fall through as Long microseconds; only TIMESTAMP_NANO uses the common BigInteger representation. After a partition field is promoted from DATE to ordinary TIMESTAMP and dropped, an old data file and a current equality-delete file therefore produce unequal keys for DeleteFileIndex, so the delete can be omitted. Convert ordinary TIMESTAMP to the same lossless representation and add a dropped DATE-to-TIMESTAMP equality-delete regression.
There was a problem hiding this comment.
Fixed in 68ef8c2. Dropped DATE, microsecond TIMESTAMP and nanosecond TIMESTAMP keys now use the same lossless BigInteger nanosecond representation, so comparisons and hashes agree without overflowing the source ranges. The dropped DATE-to-microsecond partition test now retains its equality delete, and unit tests compare keys across all three representations. All 253 related FE tests and full FE packaging/Checkstyle pass.
| throw new IllegalArgumentException("Cannot adapt partition value from " + source + " to " + target); | ||
| } | ||
|
|
||
| private abstract static class FileView<F extends ContentFile<F>> implements ContentFile<F> { |
There was a problem hiding this comment.
[P2] Keep adapted Iceberg files serializable for task reuse. ManifestCacheValue.forScan returns DataView/DeleteView when a historical manifest partition type differs from the rebound spec, but FileView implements only ContentFile and is not Serializable. Non-batch manifest planning serializes each BaseFileScanTask; this throws and planFileScanTaskWithoutReuse falls back to a full SDK scan, so adapted DATE/TIMESTAMP or dropped-field tables bypass statement manifest/task reuse. Make the views serializable with serializable state, or materialize concrete serializable Iceberg file objects, and add a non-batch adapted-task reuse regression test.
There was a problem hiding this comment.
Fixed in 68ef8c2. The shared static file-view base and both concrete wrappers are serializable with explicit serialVersionUID; their only state is the native Iceberg content file and query-owned PartitionData. No adapter, cache loader or statement resource is captured. A real supported int-to-long partition evolution test reproduced NotSerializableException before the change and now verifies serialized data/delete partition values and one manifest load across two equivalent non-batch relations. All 253 related FE tests and full FE packaging/Checkstyle pass.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #69986 at head 68ef8c2 against base a623df2 is converged after two bounded rounds. The complete changed-file set, authoritative patch, required guides, existing review threads, and added tests were inspected. No user-provided focus was supplied.
Verified findings:
- Hive streaming now records a cross-partition producer batch size as a per-partition split estimate. This makes numApproximateSplits and TSplitSource.numSplits deterministically under- or over-estimate actual work, affecting BE scanner-count and file-metadata-cache heuristics even though split delivery remains complete.
- The new global external scan worker and queue settings accept non-positive startup values. Such values pass ConfigBase parsing but fail FairTaskExecutor construction or the first per-query group creation instead of being rejected at the configuration boundary.
Concurrency, cancellation, lease ownership, FairTaskExecutor admission, bounded split queues, lazy FileSplitter parity, Iceberg prefetch/adaptation, Hudi sync publication, legacy assignment callers, and session-variable validation were rechecked. Suspected scheduler and lease races were dismissed after tracing their production lifecycles. Existing P1/P2 threads for Iceberg and Hudi were independently confirmed fixed at this head and are not duplicated. The review is static-only: no builds or tests were executed; added tests and diff checks were inspected.
Review converged after 2 round(s).
| batchException.set(new UserException(e.getMessage(), e)); | ||
| } finally { | ||
| splittersOnFlight.release(); | ||
| if (batchException.get() != null) { |
There was a problem hiding this comment.
[P2] Keep the estimate per partition. getNextBatch() advances through fileCaches/partitions whenever a range iterator is exhausted, so one returned batch can contain ranges from many partitions; this line records that combined batch size as numSplitsPerPartition. With one partition containing N ranges and splitBatchSize=1, numApproximateSplits() reports only the initial 10-split floor instead of N when N is larger; with several small partitions it can over-report. The estimate becomes TSplitSource.numSplits and drives BE scanner-count/file-meta-cache heuristics. Track per-partition counts or maintain a separate batch-size metric.
There was a problem hiding this comment.
Fixed in 663c74d. The streaming source now counts splits for its current selected partition across producer batches and files. It publishes the existing global maximum at a yield and before advancing partitions, then resets only at that boundary. There is no per-range atomic update or eager materialization. The ten-split floor and int saturation remain. Tests reproduced 17 expected versus 10 actual for a one-range batch, and 51 versus 87 when unequal partitions share a large batch; both now pass. Counts remain approximate while unseen files are still being lazily generated. All 147 related FE tests and full FE packaging/Checkstyle pass locally.
|
|
||
| @ConfField(description = "Workers for fair split production") | ||
| public static int external_scan_split_threads = 64; | ||
|
|
There was a problem hiding this comment.
[P2] Validate these global scheduler values as positive at configuration load. A non-positive external_scan_split_threads or external_scan_metadata_threads is accepted by ConfigBase and then makes ExternalMetaCacheMgr construction throw in FairTaskExecutor; external_scan_task_queue_capacity <= 0 reaches newGroup() and fails the first external scan. Add startup range validation (and a regression for invalid fe.conf values) so a typo is rejected at the configuration boundary.
There was a problem hiding this comment.
Fixed in 663c74d. All three settings opt into a pure positive-integer config validator during both ordinary and custom startup-file loading. Zero/negative values are rejected with the config name before that invalid field is updated. The existing empty-entry default rule and runtime immutability are preserved; unrelated runtime callbacks with side effects are still not dispatched during startup. Scheduler constructor/group preconditions remain for direct programmatic callers. Config tests cover both startup paths, 0/-1, valid positive/whitespace values, previous-value preservation and runtime update rejection. All 147 related FE tests and full FE packaging/Checkstyle pass locally.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: Hive streaming batches may span several partitions and a single partition may span many batches. Using the outgoing batch size as a per-partition split estimate therefore undercounts large partitions and overcounts several small ones. Accumulate generated splits locally for the current selected partition, publish the existing global maximum at yields and partition boundaries, and reset only when advancing partitions. Keep lazy enumeration, the ten-split floor and int saturation. The three new external scan scheduler sizes also accepted nonpositive startup values, failing later in scheduler construction or the first query group. Add a pure positive-integer validator to ordinary and custom startup configuration loading while preserving empty defaults, immutability and the exclusion of runtime callbacks with side effects. ### Release note Correct Hive split estimates during streaming planning and reject nonpositive external scan scheduler sizes at startup. ### Check List (For Author) - Test: Unit tests and packaging - Reproduced both configuration load failures and both split-estimate failures before repair. - All 147 related FE tests passed: 13 fe-common and 134 fe-core tests. - Full FE packaging and Checkstyle passed; git diff --check passed. - No local complete P0 or external-service regression in this cycle. - Behavior changed: Yes. Partition estimates accumulate independently of outgoing batch sizes, and nonpositive scheduler sizes are rejected during startup before the invalid field is updated. Lazy enumeration and existing scheduling/cancellation behavior are preserved. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed the complete PR diff at head 663c74d against base a623df2. The two-round review covered the fair executor and split backpressure/lifecycle paths, Hive/Hudi mode selection and cleanup, Iceberg manifest streaming and historical partition adaptation, configuration and session-variable forwarding, and all changed tests/regression files.
Verified findings are inline below: a cancellation window leaks an independently retained Iceberg table-generation lease, split_batch_size allows an unbounded producer ArrayList allocation in Hive and Iceberg, and Hive COUNT(*) streaming changes the scan-wide split decision to per-partition and can multiply Parquet/ORC metadata reads. Existing P1 threads were independently rechecked and are fixed at this head, so no existing blocking comment IDs are carried forward. The supplied review focus has no additional points.
The review converged after 2 rounds. Static-only constraints were honored: builds and tests were not executed; changed tests and regression coverage were inspected.
Review converged after 2 round(s).
| if (opt.isPresent()) { | ||
| splitAssignment.setException(new UserException(opt.get().getMessage(), opt.get())); | ||
| } else { | ||
| splitAssignment.setException(new UserException(e.getMessage(), e)); |
There was a problem hiding this comment.
[P1] Release the retained Iceberg generation when admission races cancellation. doStartSplit retains generationLease before evaluating getSplitTaskExecutor() here; if statement cleanup closes scanTaskLease in that window, Lease.group(false) throws before SplitProducer.start() registers the source and before BatchSource.close() can release generationLease. The statement's original lease is a separate reference, so repeated cancelled scans retain retired table generations/FileIO. Acquire the executor first or close generationLease in a failure path around producer construction.
There was a problem hiding this comment.
Fixed in 7e8a562. The executor/group lookup now happens before retaining the independent generation lease. Cancellation during that lookup therefore cannot strand an unregistered lease; after acquisition, SplitProducer still owns cleanup on executor rejection, cancellation and normal completion. A production-entry regression reproduces the distinct statement-reference release followed by admission failure (one leaked reference before the fix, zero after). All 218 related FE tests and full FE packaging/Checkstyle pass locally.
| for (int i = 0; i < producers; i++) { | ||
| new SplitProducer(splitAssignment, executor, | ||
| createSplitBatchSource(cache, nextPartition, broker, numBackends), failure -> { | ||
| try { |
There was a problem hiding this comment.
[P1] Bound the producer batch allocation independently of the user setting. split_batch_size accepts any positive int, and this new ArrayList<>(splitAssignment.getSplitBatchSize()) allocates that many references before backpressure or file availability is known. A query hint such as split_batch_size=2147483647 can trigger an FE OOME (the Iceberg batch source has the same allocation). Cap the setting or start with a small bounded capacity and grow as batches are filled.
There was a problem hiding this comment.
Fixed in 7e8a562. Both Hive and Iceberg now use one batch allocator that limits only the initial ArrayList capacity to 128, retaining the configured batch maximum and growing with actual work. An isolated 64MB JDK probe reproduced Integer.MAX_VALUE failing before any split. Actual-source tests now cover that maximum with 257 Hive ranges (no artificial 128-range cap) and empty Iceberg planning with generation cleanup. All 218 related FE tests and full FE packaging/Checkstyle pass locally.
| private Iterator<Split> ranges = Collections.emptyIterator(); | ||
| private FileCacheValue currentCache; | ||
| private long targetSize; | ||
| private boolean needSplit; |
There was a problem hiding this comment.
[P2] Keep the COUNT() split decision scan-wide in streaming mode. createSplitBatchSource now calls needSplitFiles(loaded, numBackends) after loading each singleton partition, so a table with 100 one-file partitions and a parallelism threshold of 32 makes every 1-GB Parquet/ORC file splittable even though the aggregate file count would disable splitting in the eager path. Each file is then split into many ranges and its metadata is reread, defeating the count-pushdown optimization. Carry the aggregate selected-file count (or an equivalent shared decision) while retaining lazy range materialization, and add a multi-partition COUNT() regression.
There was a problem hiding this comment.
Fixed in 7e8a562. COUNT(*) now makes one scan-wide decision using bounded metadata lookahead on the existing concurrent, cancellable producers. They retain nonempty file metadata until the parallelism/backend threshold is reached, allowing only already in-flight partitions to complete, then share that decision and reuse the metadata before lazily listing further partitions. Singleton listing and original interruption/fair scheduling are preserved; no split-executor task waits for child tasks. The 100-file regression produced 800 ranges before repair and 100 after; a two-file scan still splits to 16 ranges. Tests cover both scanner versions, Parquet/ORC, disabled file caching without relisting, bounded lookahead, metadata failure and cancellation. All 218 related FE tests and full FE packaging/Checkstyle pass locally.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: Cancelled Iceberg planning can retain an independent table-generation lease when task-group lookup throws before producer cleanup is registered. Resolve that executor before acquiring the lease. Hive and Iceberg also eagerly allocate a configured maximum batch capacity, so a large valid setting can fail with OOM even for empty work; share a bounded initial allocation while preserving the configured batch maximum. Hive streaming COUNT(*) decides whether to split one partition at a time, producing excessive ranges and footer reads when the selected scan already has enough files. Use the existing concurrent cancellable producers for bounded metadata lookahead, make the scan-wide decision after all lookahead workers finish, and reuse captured metadata before lazily listing further partitions. Preserve singleton listing, fair task admission, interruption, range backpressure and the eager COUNT policy. ### Release note Avoid excessive Hive COUNT(*) scan ranges, premature memory allocation for large split batch settings, and leaked Iceberg planning references after cancellation. ### Check List (For Author) - Test: Unit tests and packaging - Reproduced the Iceberg lease leak and Hive COUNT range regression before repair; isolated the allocation failure in a separate 64MB JDK process. - All 218 selected FE tests passed with no failures, errors or skips, using run-fe-ut.sh. - Covered both scanner versions, Parquet/ORC, metadata reuse without file caching, few-file splitting, maximum batch settings, metadata failure and lookahead cancellation. - Full FE packaging and Checkstyle passed using build.sh --fe -j4; git diff --check passed. - Complete P0 and external-service regression were not run locally in this cycle. - Behavior changed: Yes. Streaming COUNT(*) uses the selected scan's split decision, large batch limits allocate only for actual work, and failed admission acquires no independent generation reference. Scheduling and cancellation continue through existing producers. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR 69986 at head 7e8a562 found two verified P1 issues. First, the new lazy FileSplitter iterator catches checked IOException even though SplitCreator.create and every other call in the try body are non-throwing; javac rejects the catch, so the FE module cannot compile. Second, deferred Arrow Flight Hudi batch scans register BatchFsViewOwner as a statement resource whose close unconditionally stops SplitAssignment; StatementContext cleanup after GetFlightInfo therefore stops the lazy SplitSource before the BE's later DoGet fetch, despite the new scan-level deferred cleanup guard. The existing P0/P1 inline threads were revalidated against this head and all are fixed, so existing_blocking_comment_ids is empty. The review focus file contains no additional user focus. The changed scheduler, Hive streaming/count lookahead, Hudi lifecycle, Iceberg manifest prefetch/cache adaptation, configuration, and tests were inspected statically; no builds or tests were run under the static-only contract. Delivery is non-converged at the three-round cap: the main agent completed the Iceberg sweep and found no additional issue, but the required recovery and risk-focused child reviews could not run because model capacity errors prevented their startup. The two verified findings are delivered; this missing child-agent evidence is the remaining convergence limit.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
At the three-round cap, two required child scopes (Iceberg recovery and the separate risk-focused recheck) could not start because the model-capacity error recurred. The main agent independently swept those files and found no additional issue, but the missing child-agent results prevent convergence under the bounded review contract.
| } | ||
| } | ||
| return result; | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
[P1] Remove the unreachable checked IOException catch in splitFileIterator's anonymous Iterator.next(). SplitCreator.create(...) has no throws IOException, and every other call in that try body is non-throwing, so javac reports exception IOException is never thrown in body of corresponding try statement and the FE module cannot compile. Either remove the catch/wrapper or change the SplitCreator API and all implementations to a checked contract.
There was a problem hiding this comment.
This finding does not apply to reviewed head 7e8a562. The try body also calls Hadoop BlockLocation.getHosts() in the unsplittable and explicit-size branches; the actual Hadoop 3.4.2 dependency declares public String[] getHosts() throws java.io.IOException (confirmed with javap). That checked exception must be adapted because Iterator.next cannot declare it. SplitCreator.create need not throw. The exact-head remote COMPILE build 1089472 succeeded, and local full FE packaging/Checkstyle also passed. I retained this catch and the lazy UncheckedIOException/eager IOException boundary. The repair head f69a307 again passes all 90 selected FE tests and full FE packaging/Checkstyle.
| protected SplitAssignment createSplitAssignment(Map<String, String> properties, | ||
| List<String> partitionKeys, boolean admission) { | ||
| // Hudi still submits eager partition batches and must not block on Hive's small queues. | ||
| return new SplitAssignment(backendPolicy, this, this::splitToScanRange, |
There was a problem hiding this comment.
[P1] Keep deferred Arrow Flight Hudi split owners alive until coordinator finalization. FileQueryScanNode now dispatches batch scans through this Hudi-specific assignment factory, but Hudi registers BatchFsViewOwner as a StatementContext resource and its close() unconditionally calls splitAssignment.stop(). StatementContext closes resources at the end of GetFlightInfo, so a Hudi batch scan is stopped before the BE's later DoGet/fetchSplitBatch, even though FileQueryScanNode explicitly preserves deferred scans. Make this owner cleanup defer-aware while retaining cancellation/connection teardown, and add a Hudi deferred-batch regression.
There was a problem hiding this comment.
Fixed in f69a307. Hudi now registers a statement-only cleanup wrapper that captures this query executor and checks its deferred flag when the statement closes. The actual BatchFsViewOwner remains an unconditional SplitAssignment resource, so coordinator finalization/query cancellation still cancels tasks and releases its lease after terminal accounting. Capturing the executor preserves the handoff even if processor cleanup clears it from ConnectContext. A regression using real HudiScanNode.startSplit, StatementContext, SplitAssignment and source registration reproduced premature stop before repair; it now retains the queued producer/source through deferred statement close, then verifies explicit scan stop cancels the producer, removes the source and releases the lease exactly once. Ordinary and absent-executor statement cleanup remain covered. All 90 selected FE tests and full FE packaging/Checkstyle pass locally; no live Flight/Hudi external regression is claimed.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: Hudi batch planning registers its filesystem-view owner directly as a statement resource. Closing the statement after GetFlightInfo therefore stops SplitAssignment and cancels its producer before a deferred Arrow Flight query can fetch splits during DoGet. The existing scan-level deferred cleanup guard cannot protect this independently registered owner. Register a statement-only cleanup wrapper that captures the query executor and checks its deferred flag at close time, while keeping the actual owner as an unconditional SplitAssignment resource for coordinator finalization and query cancellation. Preserve duplicate registration failure cleanup, task accounting, interruption and exact terminal lease release. The captured executor remains valid even if connection cleanup clears its current executor reference. ### Release note Keep deferred Hudi Arrow Flight batch scans alive until query finalization or cancellation. ### Check List (For Author) - Test: Unit tests and packaging - Reproduced the premature assignment stop through real HudiScanNode.startSplit and StatementContext.close before repair. - Added deferred, ordinary and absent-executor controls; verified registered source lifetime, forced scan stop, queued producer cancellation and one lease release. - All 90 selected FE tests passed with no failures, errors or skips using run-fe-ut.sh. - Full FE packaging and Checkstyle passed using build.sh --fe -j4; git diff --check and final source hashes passed. - No live Arrow Flight/Hudi external-service regression was run locally in this cycle. - Behavior changed: Yes. Deferred Hudi scans survive statement cleanup, while non-deferred cleanup and explicit query cancellation still stop the batch owner. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static exact-head review of PR #69986 at head f69a307 against base a623df2. I inspected the complete changed-file list, authoritative diff, required AGENTS.md and code-review guidance, existing review threads, the empty user focus, and the shared ledger. The main sweep covered the FairTaskExecutor and ScanTaskGroups lifecycle, SplitAssignment/SplitSource/SplitProducer queue admission and backpressure, FileQueryScanNode stop and ownership paths, Hive/Hudi streaming and count-mode decisions, FileSplitter iteration and estimates, session/config forwarding, and Iceberg manifest prefetch, cache/generation leases, delete-file pairing, partition adapters, and serialization. No new verified inline finding remains at this head. Existing P1/P2 threads were rechecked against the current code and are fixed or no longer applicable, including the generation lease admission, bounded batch allocation, COUNT(*) decision, FileSplitter checked-exception, Hudi deferred cleanup, estimate, and Iceberg adapter/temporal issues; therefore existing_blocking_comment_ids is empty and no duplicate comments are posted. The review is static-only: builds and tests were not executed, while changed tests and their coverage were inspected. The required normal and separate risk-focused child reviews could not return ledger evidence: the initial attempts and the requested gpt-6-sol recovery attempts terminated with model-capacity errors or no output. The main agent independently completed the final changed-file, candidate, risk, and live-thread sweep, but the missing child results prevent convergence under the contract. This is the third and final bounded round; no fourth round was started.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The required normal child reviews and separate risk-focused recheck did not produce ledger results. Initial and gpt-6-sol recovery attempts terminated with model-capacity errors or no output. Main-agent coverage and live-thread revalidation are complete, but child evidence is required for convergence; the three-round cap has been reached.
…nning ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: External metadata planning can monopolize shared workers and retain eagerly planned split graphs. Backport bounded lazy production and fair scheduling to branch-4.2, retaining generation ownership and previous review fixes. Integrate with the release branch's individual scan-range queues, per-backend cancellation, live empty-batch protocol, and Iceberg partition/cache versioning. Transfer Hudi partition admission and filesystem-view ownership through asynchronous delivery so queue backpressure neither blocks workers nor permits unbounded pending partitions. ### Release note Bound and fairly schedule external scan planning. ### Check List (For Author) - Test: 386 focused FE unit tests passed through run-fe-ut.sh; full FE packaging and integrated Checkstyle passed through build.sh --fe -j4. External-service regression was not rerun locally. - Behavior changed: Yes. External scan planning uses bounded lazy production and fair scheduling while preserving branch-4.2 delivery semantics. - Does this need documentation: No
f69a307 to
b52f1af
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: HiveSplitEstimateTest still mocks the former addToQueue delivery path. The fair producer reads a zero batch size from that mock and emits no files, so four current-head FE tests fail before verifying their estimates. Use a real SplitAssignment and SplitSource with deterministic backend assignment, retain the existing file-count and estimate assertions, and stop the assignment during cleanup. ### Release note None ### Check List (For Author) - Test: Reproduced four failures locally; all 35 focused FE tests passed through run-fe-ut.sh. Full FE packaging and integrated Checkstyle passed through build.sh --fe -j4. - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69298, apache#69986 Problem Summary: The cold file-count regression requests EXPLAIN VERBOSE while asserting approximate streaming split counts. Branch-4.2 deliberately disables batch mode for VERBOSE so it can enumerate exact file details. Use normal EXPLAIN for this streaming-selection assertion, matching the suite's other streaming checks, and retain all expected data results. ### Release note None ### Check List (For Author) - Test: 21 focused Hive FE tests passed through run-fe-ut.sh, covering exact VERBOSE mode and cold streaming selection; git diff --check. External Hive regression is not available locally and must be verified by remote CI. - Behavior changed: No - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: apache#69298 Problem Summary: External split production and Iceberg manifest reads moved from the registered scheduleExecutor to FairTaskExecutor instances constructed directly by ExternalMetaCacheMgr. The existing thread_pool metrics therefore no longer showed the migrated workers, queued work or progress. A production-entry unit test reproduced zero registered NotCheckpointExternalSplit pool-size metrics before the fix. Create these schedulers through ThreadPoolManager and include their existing counters in its startup metric registration, using distinct normal and checkpoint names. Latch-controlled real scheduler tests verify active, queued, completed and group values through MetricRepo. Scheduling and delivery behavior are unchanged. ### Release note External split and metadata schedulers now expose thread_pool metrics for worker count, active tasks, queued tasks, completed tasks and task groups. ### Check List (For Author) - Test: Unit Test and build - 45 focused FE tests passed with zero failures, errors or skips via run-fe-ut.sh; the new production-entry metric test failed before the fix and passes after it. - build.sh --fe -j4 including Checkstyle passed. - Behavior changed: Yes, export metrics for the replacement schedulers; query scheduling and scan delivery are unchanged. - Does this need documentation: No, existing thread_pool metric labels are reused.
|
run buildall |
|
/review |
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69986
commit: e878673f7dd827df52207b736bf8359e5413cd94
base: 914257a1fd054265225bb19637546f5ab659ad4a
reviewed_at: 2026-11-03T18:02+08:00
reviewer: 924060929
model: gpt-6.1-sol
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
1 similar comment
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
PR approved by at least one committer and no changes requested. |
cherry pick from #69298