Repository navigation
[fix](fe) Guard stream load schema metadata reads - #68964
Merged
Merged
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68726 Problem Summary: Concurrent schema change can remove base-index metadata before switching the base index ID. Protect regular and HTTP Stream Load result metadata reads with the table read lock. Abort the already-begun normal HTTP transaction if the metadata lock times out; group commit does not begin that transaction. Keep the original timeout and record cleanup failures. ### Release note Prevent Stream Load schema-version races and clean up HTTP transactions on metadata lock timeout. ### Check List (For Author) - Test: Standard FE UT reactor passed StreamLoadHandlerTest (10) and HttpStreamLoadTablePropertiesTest (5), 15 tests with zero failures/errors. Maven Checkstyle and git diff --check passed. - Behavior changed: Yes. Bound metadata read-lock waiting by RPC timeout and abort normal HTTP transaction on timeout. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
5 of 16 tasks
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68964 Problem Summary: Add regression coverage for regular Stream Load and HTTP load_sql metadata reads during schema change, including HTTP Group Commit. Add unit coverage for lock timeout and exceptional metadata reads so the read lock is not bypassed and is always released. ### Release note None ### Check List (For Author) - Test: 17 focused FE unit tests, Checkstyle, quick checks, new HTTP Stream Load P0 suite snapshot generation and comparison, existing schema-change P0 suite, and existing HTTP Stream Load properties P0 suite; all passed. - Behavior changed: No. - Does this need documentation: No
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28644 ms |
Contributor
TPC-DS: Total hot run time: 151495 ms |
Contributor
ClickBench: Total hot run time: 24.35 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Collaborator
Author
|
run feut |
924060929
previously approved these changes
Oct 13, 2026
Contributor
FE UT Coverage ReportIncrement line coverage |
Gabriel39
pushed a commit
that referenced
this pull request
Oct 13, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: #68964 Problem Summary: Repeated queries against a single remotely accessible file split can repeatedly select the same backend when external file caching and consistent-hash scheduling are disabled. The round-robin scheduler reconstructed its candidate list from a host map, discarding the query-local randomized order selected during initialization. This change preserves that order for round-robin assignment and keeps local scheduling and split redistribution inside the active backend candidates. For unfiltered, ungrouped `COUNT(*)` and `COUNT(1)` over File, HDFS, HTTP, Local, and S3 TVFs, the existing file COUNT pushdown is now used under the existing session switches. The instance reduction is limited to a file TVF row-count scan with exactly one assigned static physical split. Dynamic `split_source` entries, multiple physical splits in one range envelope, and existing external-table COUNT paths retain their original parallelism. DISTINCT, grouped aggregates, filtered plans, merge stages, and stream/ingest TVFs remain outside this path. The single-split CSV COUNT benchmark improved median QPS from 169.600 to 543.733 (3.206x) under the conditions below. Dynamic batch scans retain baseline fragment-instance parallelism. Stream Load schema-version locking has been removed from this PR: a differential unit test reproduces the race on the pre-optimization baseline. It is handled separately in #68964, including HTTP transaction cleanup on table-lock timeout. This PR retains the external scan scheduling and file TVF COUNT changes. ### Release note Improve round-robin scheduling for external file scans and reduce execution overhead for unfiltered file TVF row-count queries with a single assigned static physical split. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason Validation performed: - After separating the independent Stream Load schema-lock fix, the standard focused FE UT reactor passed 33 tests: `FederationBackendPolicyTest` (12), `FileTvfCountPushDownTest` (5), `UnassignedScanSingleRemoteTableJobTest` (7), and the upstream `StreamLoadHandlerTest` (9). Maven Checkstyle passed. This is functional validation of head `0afd4837804`; the benchmark numbers below are from the earlier measured revision. - Standard full FE packaging and Checkstyle passed. Standard focused FE UT reactor passed: `FederationBackendPolicyTest` (12), `FileTvfCountPushDownTest` (4), and `UnassignedScanSingleRemoteTableJobTest` (7), 23 tests total, zero failures/errors. Parallelism tests cover dynamic split sources, multiple physical splits in a single range envelope, external-table exclusion, and actual local-shuffle instance assignment. - New `test_file_tvf_count_pushdown` passed both framework snapshot generation and normal comparison (1 suite, zero failures/fatal scripts). It exercises CSV, JSON, Parquet, and ORC with scanner V1/V2 and pushdown enabled/disabled, including COUNT star/one/nullable/DISTINCT, filters, grouping, empty inputs, multiple files, and CSV headers/blank records/no final newline. Zero-byte JSON is included alongside nonempty files because standalone schema inference is unavailable. - `test_s3_tvf_compression` passed against the personal MinIO equivalent using the same public compression fixtures (1 suite, zero failures/fatal scripts). The standard Aliyun OSS profile remains unvalidated because valid credentials are unavailable; community CI should rerun it with its configured credentials. - Full Release BE build passed. Both FE versions were tested with identical newly built BE binaries; the new four-format and compressed TVF suites were also repeated successfully on this runtime. - Single static split CSV (`COUNT(*)`, 1,000,000 rows, 64 clients): three baseline QPS values were **165.533 / 170.800 / 169.600**; candidate values were **449.133 / 544.967 / 543.733**. Median QPS increased from **169.600 to 543.733 (3.206x)**. This measures the combined scheduling and COUNT changes for this workload; it does not establish a scheduling-only multiplier. - Real dynamic Iceberg batch (`COUNT(value)`, 64 data files, 1,000,000 rows, expected count 500,000, 16 clients): baseline QPS **29.533 / 30.367 / 28.233**, candidate **30.167 / 29.767 / 29.267**; medians **29.533 / 29.767 (1.008x)**. Distributed plans assigned **192 scan fragment instances, 64 per BE**, for both versions in all rounds, confirming dynamic sources keep their baseline instance parallelism. These are assigned fragment instances, not active scanner-thread counts. This throughput comparison is limited to the tested fixture and its observed variability. - All twelve measurements had verified COUNT results, zero errors, and 3/3 post-measurement backend health. Final artifact inspection found ten connector/dependency JARs with an older API-version manifest because size/mtime-based synchronization skipped them. ZIP entry comparisons confirmed identical classes/resources; the Iceberg provider JAR itself and FE core matched the intended builds. API gating reads the provider's defining JAR, so the differing SPI dependency manifest is outside this check. The measurements are therefore a comparison of the tested workload paths, not a claim of identical complete FE packaging. Unrelated ADBC/ES/Hudi/Trino providers were rejected during startup. A subsequent checksum deployment matched all 2,213 FE artifact hashes and all three BE binaries to the frozen build manifest. All previously rejected providers and Iceberg loaded successfully; static COUNT=1,000,000, dynamic nullable COUNT=500,000, and 3/3 health passed. This deployment smoke did not rerun the performance measurements. - Manual benchmark procedure: alternate baseline/candidate FE builds in three rounds (B/C, C/B, B/C), restart all three isolated BEs after each FE switch, require 3/3 health and the expected COUNT result, warm up for 10 seconds, then measure verified successful queries over 30 seconds. Use identical Release BE binaries and session settings (`parallel_pipeline_task_num=64`, file cache/consistent hash/SQL cache/query cache disabled, COUNT pushdown and scanner V2 enabled). Use 64 clients for the single-split CSV and 16 for the dynamic Iceberg batch; keep profiling disabled during measurement. All measurements require zero errors. The three-BE restart prevents stale cached FE RPC connections after version swaps; startup failures are excluded before measurement. - Benchmarks use three BEs on one shared host and report only the tested workload; no equivalent multiplier is claimed for all queries or customer deployments. - Behavior changed: - [ ] No. - [x] Yes. Round-robin assignment preserves initialized candidate order; preferred load subsets are honored by local scheduling and redistribution; eligible file TVF COUNT scans use pushdown and one instance per worker when that worker has exactly one static physical split; dynamic sources and existing external-table COUNT parallelism remain unchanged. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68964 Problem Summary: Stream Load response metadata reads can race with concurrent base-index replacement. Populate the first table's response metadata under the existing regular planning lock and use the generated sink metadata for HTTP load_sql. Remove the additional lock acquisition and the transaction cleanup branch introduced for its timeout. Preserve multi-table behavior and protocol fields. ### Release note Avoid Stream Load schema-version races without additional metadata lock waiting. ### Check List (For Author) - Test: Standard FE UT reactor passed 16 tests (14 StreamLoadHandlerTest and 2 HttpStreamLoadTablePropertiesTest). Standard ASAN FE/BE build and Checkstyle passed. Three targeted P0 suites passed: test_stream_load_schema_metadata, test_http_stream_properties, test_schema_change_unique_mow. Source diff whitespace check passed. - Behavior changed: Yes. Response metadata uses the existing planning lock or generated sink. - Does this need documentation: No
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29347 ms |
Contributor
TPC-DS: Total hot run time: 152576 ms |
Contributor
ClickBench: Total hot run time: 24.48 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
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.
Reviewed all six changed files and their relevant FE/BE planning, schema-change, transaction, and test-framework paths at ed917f9. No actionable new findings or existing blocking inline comments were identified. The review converged in round 1: both complete-scope reviewers and the additional risk reviewer returned NO_NEW_VALUABLE_FINDINGS, and all candidates and risk focuses were independently verified and disposed of.
The static evidence supports the fix: regular Stream Load captures response schema metadata within its existing planning read lock; HTTP load_sql copies metadata from the sink initialized under the target-table planning lock. Both avoid the old-index deletion/base-index switch window without another lock acquisition.
Critical checkpoint conclusions:
- Goal and proof: the protected reads address the concrete schema-replacement race. Added latch-based unit coverage exercises replacement; HTTP helper coverage verifies planned values remain usable when live base-index metadata is absent. Tests were inspected, not executed.
- Scope and clarity: the two production changes are focused metadata relocation/helper extraction, with corresponding unit and regression coverage.
- Concurrency: the Stream Load RPC thread's table read lock excludes the schema-change worker's write-locked old-index deletion/base-id switch. HTTP sink finalization captures version and table id under the target read lock. New locked work is constant-time assignment, with no added IO, RPC, lock acquisition, or lock-order change.
- Lifecycle: request-local response/fragment state and saved thrift payload have clear ownership. Existing finally blocks release acquired locks on errors; timeout does not unlock an unacquired lock. No new static initialization or resource lifecycle.
- Configuration: no new configuration, defaults, or dynamic-update behavior.
- Compatibility: existing response fields, public interfaces, and storage formats remain compatible; no new rolling-upgrade dependency.
- Thrift/protobuf: no definitions, field IDs, enum values, or required fields change.
- Parallel paths: regular/cloud planners, HTTP ordinary and explicit/internal Group Commit, first-table response semantics, and null-response multi-table callers were traced. GroupCommitBlockSink retains its inherited OLAP payload and finalization even with txn=-1. Empty plans fail existing HTTP validation before response extraction.
- Conditions: the non-null response and empty-fragment-list guard has concrete no-response/first-table purposes, with direct tests. Failure continues to return non-OK RPC status before BE execution.
- Test coverage: positive replacement, timeout, exceptional unlock, first-table and no-response unit cases are present. Regression covers regular/HTTP loads across replacement and addition, plus HTTP sync Group Commit. Actual HTTP Group Commit follows its existing load_sql path and does not encounter the regular light-schema eligibility gate.
- Test results: ordered golden rows match repeated same-key MoW upserts and DEFAULT 7 for the added column. The suite drops before use and preserves its table. Actual golden generation and runtime pass were not independently verified.
- Observability: existing SQL/query/label/transaction logs and error conversion cover the affected paths; no new operational stage needs metrics.
- Transactions, persistence, and failover: transaction index/partial-update capture remains protected; begin, abort, commit, publish, journal, and replay behavior are unchanged. Response metadata adds no persisted state or new failover phase.
- Writes, atomicity, and crashes: no new data-write or visibility transition is introduced. Committed/uncommitted visibility, partition-version authority, and MoW delete-bitmap behavior are unaffected. No new BE memory allocation, cleanup duty, leak, or deadlock mechanism was found.
- FE-BE passing: existing db/table/schema-version fields are populated on the inspected response paths; HTTP uses the saved sink snapshot. BE Group Commit still matches sink schema version/index count and rejects incompatibility.
- Performance: no additional metadata lock, scan, collection allocation, or RPC; the helper reads saved scalar values.
- Other issues and user focus: no additional user-provided focus or further actionable defect. No inline duplicates were posted.
Review limits: this was static-only, with no builds or tests. Planner/payload mocks do not establish executed integration or cloud/failover coverage, and the HTTP regression does not deterministically schedule the exact race. Arbitrary multi-fragment HTTP SQL retains a pre-existing one-fragment dispatch limitation; it is not a newly introduced regression or an unresolved changed-code candidate. All six changed files and assigned risks are covered, with no unresolved review coverage gap.
Review converged after 1 round(s).
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:
Stream Load reads the base schema version from table metadata outside the planning lock. Concurrent schema change removes the old base-index metadata before switching the base index ID under the table write lock. An unlocked read in that interval can dereference missing metadata and fail the load. A deterministic unit test reproduces the null dereference on the pre-optimization baseline.
For regular Stream Load, populate the response database ID, table ID, and base schema version inside the existing
generatePlan(OlapTable)read-lock scope. Populate them only for the first planned table when a response is present, preserving multi-table behavior. Planning and response metadata use the same lock scope without an additional lock acquisition.For HTTP
load_sql, obtain the response table ID and base schema version from the generatedOlapTableSink. These values were already captured during planning under the existing table lock. Keep the other table-property reads as before. This removes the second metadata-lock acquisition and the transaction-abort branch introduced solely for its timeout. Existing transaction handling and Thrift fields are preserved.This fix is separate from the scheduling and file TVF COUNT optimization in #68726.
Release note
Prevent Stream Load response schema-version reads from racing with schema change by reusing the existing planning lock and planned sink metadata.
Check List (For Author)
Test
Validation performed:
StreamLoadHandlerTest(14) andHttpStreamLoadTablePropertiesTest(2): 16 tests, zero failures/errors. Maven Checkstyle passed.load_p0/http_stream/test_stream_load_schema_metadataandload_p0/http_stream/test_http_stream_propertiespassed with zero failed suites/fatal scripts. Existing regression snapshots were compared without regeneration or manual edits.schema_change_p0/test_schema_change_unique_mowalso passed: one suite, zero failed suites/fatal scripts. All three targeted suites used the binaries built from this revised implementation. Source changes passgit diff --check.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)