Conversation
### What problem does this PR solve? Problem Summary: Packed slice collection depends only on a closed file writer, its logical path, and the destination rowset metadata, but was a private method of CloudRowsetWriter. Move it to RowsetMeta::collect_packed_slice_location and reuse it for existing Cloud segment and index collection without changing collection timing. Preserve skipping unfinished and non-packed files. Require a nonempty packed object path after a successful packed-location lookup, replacing the previous silent skip with an invariant assertion. Move the associated RowsetMetaPackedFileTest coverage with this refactor: completed data/index files, logical paths distinct from writer URIs, repeated collection, serialization, non-packed files, and missing mappings. Memtable forwarding call sites and MOW collection timing changes remain in subsequent commits. During rebase, retain the upstream writer-owned packed slice lookup so global index cleanup cannot invalidate metadata collection. Adapt the unit coverage to real PackedFileWriter handles, including incomplete writers, direct files, serialization, repeated collection, and missing-handle invariant failures. ### Release note None ### Check List (For Author) - Test: Unit tests adapted but not run, per request. No build or service execution. Conflict-resolution files passed clang-format 16, format checks, build hygiene, and Git whitespace checks. - Behavior changed: Yes; an empty packed object path after successful lookup is an invariant failure instead of a silent skip - Does this need documentation: No
### What problem does this PR solve? Problem Summary: Cloud MOW loads build temporary rowsets in asynchronous delete bitmap tasks while other flushes can still add or close file writers. Building those temporary rowsets previously traversed all segment and index writer collections without their locks to collect packed mappings. Collect only the current segment's mapping after its writer closes, and reserve full collection for final rowset construction after flushes finish. Pass the segment ID through _build_tmp and update the existing unit-test wrapper and call accordingly. CloudRowsetWriter::build also collected the same mappings a second time directly after _build_rowset_meta had already collected them. Remove that redundant call. These issues affect existing Cloud load paths independently of memtable forwarding. Forwarded-file snapshots and direct-upload logic remain in the following feature. ### Release note Avoid concurrent traversal of file writer collections during Cloud MOW packed file loads and eliminate duplicate packed mapping collection at finalization. ### Check List (For Author) - Test: Static call-site review and git diff --check; existing TmpRowsetUsesCompletedSegmentIds test adapted to the signature change. No builds or runtime tests at user request; concurrent execution was not reproduced during this split. - Behavior changed: Yes; temporary rowsets collect only the current segment's packed mapping and final rowsets collect all mappings once - Does this need documentation: No
mymeiyi
requested review from
gavinchou,
liaoxin01 and
yiguolei
as code owners
September 22, 2026 04:00
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address transaction/vault propagation, empty packed-file handling, and unordered regression queries.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds cloud-mode memtable-on-sink support with streamed rowset handling, metadata propagation, cloud commits, and regression coverage.
Changes:
- Enables cloud sink memtables for pipeline and broker loads.
- Propagates transaction, vault, and stream metadata.
- Adds packed-file handling and unit/regression tests.
| File | Summary |
|---|---|
regression-test/suites/cloud_p0/test_cloud_memtable_on_sink.groovy |
Cloud sink-memtable tests |
regression-test/suites/cloud_p0/test_cloud_memtable_mow_forward.groovy |
MoW forwarding tests |
regression-test/data/cloud_p0/test_cloud_memtable_on_sink.out |
Expected test results |
regression-test/data/cloud_p0/test_cloud_memtable_mow_forward.out |
Expected test results |
gensrc/proto/internal_service.proto |
Load-stream metadata fields |
fe/fe-core/src/main/java/org/apache/doris/load/loadv2/BrokerLoadJob.java |
Enables cloud broker sink memtables |
be/test/storage/rowset/rowset_meta_test.cpp |
Packed-location unit tests |
be/test/storage/rowset/beta_rowset_test.cpp |
Delete-bitmap failure tests |
be/src/storage/rowset/rowset_meta.h |
Packed-location API |
be/src/storage/rowset/rowset_meta.cpp |
Packed-location handling |
be/src/storage/rowset/beta_rowset_writer.h |
Temporary-rowset API updates |
be/src/storage/rowset/beta_rowset_writer.cpp |
Streamed rowset metadata handling |
be/src/load/channel/load_stream.h |
Load metadata state |
be/src/load/channel/load_stream.cpp |
Metadata propagation |
be/src/load/channel/load_stream_writer.cpp |
Cloud rowset writing |
be/src/exec/sink/writer/vtablet_writer_v2.h |
Transaction and vault state |
be/src/exec/sink/writer/vtablet_writer_v2.cpp |
Enriched load streams |
be/src/exec/sink/load_stream_stub.h |
Stream-open interface updates |
be/src/exec/sink/load_stream_stub.cpp |
Stream request metadata |
be/src/exec/pipeline/pipeline_fragment_context.cpp |
Enables cloud sink memtables |
be/src/cloud/cloud_rowset_writer.h |
Shared metadata collection |
be/src/cloud/cloud_rowset_writer.cpp |
Packed metadata collection |
be/src/cloud/cloud_rowset_builder.h |
Cloud transaction commit API |
be/src/cloud/cloud_rowset_builder.cpp |
Cloud transaction commit |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes: the static review found four PR-attributable issues: mixed-version sink incompatibility, loss of the cloud empty-rowset metadata-skip lifecycle, serialized metadata RPCs under stream locks, and stale regression output. The two existing inline threads remain valid duplicate fences and were not repeated.
Critical-checkpoint conclusions:
- Goal and proof: the change enables memtable-on-sink for cloud loads and covers packed/MoW paths, but the checked-in tests do not prove rolling upgrades, sparse many-bucket loads, or many-tablet close behavior; one expected-output file is not the output of the current suite.
- Scope and reuse: the diff is focused, but the new receiver bypasses specialized lifecycle/orchestration already present in
CloudDeltaWriterandCloudTabletsChannel. - Concurrency: per-tablet flush ordering and MoW snapshot lifetimes are sound in the inspected paths. No new lock-order deadlock was found, but cloud sync/prepare/commit RPCs now run serially while
IndexStream/LoadStreamlocks are held. - Lifecycle and static initialization: packed writer ownership, temporary-rowset capture, cancellation, and nonempty commit/cache/stat ordering were traced and found consistent. Empty-rowset handling is not consistent with the established path. No cross-TU/static-initialization change is involved.
- Configuration: no new config was added. The existing default-enabled
skip_writing_empty_rowset_metadatasetting is bypassed by the new receiver path. - Compatibility: the optional protobuf fields are wire-compatible, but feature/protocol selection is not semantically safe during a rolling BE upgrade.
- Parallel paths and conditions: partial update, V1 inverted index, and row-binlog fallbacks remain gated. The legacy cloud path's empty handling and bounded commit concurrency were not carried into the new path.
- Tests and results: unit and Docker regression coverage was added, but it lacks mixed-version, sparse multi-bucket metadata, and many-tablet latency/fault cases.
test_cloud_memtable_on_sink.outcontains unreachable blocks and must be regenerated or matched by restored cases. - Observability: existing profiles, logs, and commit latency metrics are adequate for the reviewed flow; no separate observability blocker was found.
- Transactions, persistence, and crash behavior: no FE EditLog change is involved. Cloud prepare/build/bitmap/commit behavior was traced; apart from empty-rowset metadata and rolling-upgrade receiver aborts, no additional atomicity or cleanup defect was substantiated.
- FE/BE variables: the direct stream-open fields propagate through the reviewed path. The already-open inline thread covers the separate internal expiration/vault omission, so it is not duplicated here.
- Performance: empty metadata amplification and lock-scoped serial meta-service RPCs are blocking concerns. No additional CPU, memory, or mapping issue was substantiated.
- Other: packed metadata and MoW physical-segment ordering converged with no new finding beyond the existing zero-byte packed-index thread.
User focus: no additional focus was supplied. Validation was static only as required; no build or test was run, and author/CI claims were not treated as independent execution evidence.
| if (state->query_options().enable_memtable_on_sink_node && | ||
| !_has_inverted_index_v1_or_partial_update(thrift_sink.olap_table_sink) && | ||
| !_has_row_binlog(thrift_sink.olap_table_sink) && !config::is_cloud_mode()) { | ||
| !_has_row_binlog(thrift_sink.olap_table_sink)) { |
Contributor
There was a problem hiding this comment.
[P1] Add a mixed-version capability fence before enabling the cloud V2 sink. During a rolling upgrade this per-BE condition makes new fragment instances choose the V2/load-stream protocol while old instances choose the legacy tablets channel. A new sender can also target an old tablet-location BE; that receiver's LoadStreamWriter unconditionally calls storage_engine().to_local(), whose type check aborts in cloud mode. The optional request fields do not negotiate support. Gate this at the coordinator/minimum BE version (or negotiate and fall back) so every sender and receiver in one load uses a compatible protocol.
| // FIXME(plat1ko): No `commit_txn` operation in cloud mode, need better abstractions | ||
| RETURN_IF_ERROR(static_cast<RowsetBuilder*>(_rowset_builder.get())->commit_txn()); | ||
|
|
||
| RETURN_IF_ERROR(_rowset_builder->commit_txn()); |
Contributor
There was a problem hiding this comment.
[P1] Avoid running the cloud metadata RPCs serially under the load-stream locks. This call is reached for every tablet from IndexStream::close() while its _lock is held, and that close itself runs under LoadStream::_lock; the first append also holds the index lock across sync_rowsets() and prepare_rowset(). A many-tablet load therefore pays the sum of all prepare/commit RPC latencies and one slow tablet head-of-line blocks unrelated tablets. CloudTabletsChannel already uses batch initialization and bounded bthread_fork_join(..., 10) for this lifecycle. Move remote work out of the locks and preserve bounded concurrency.
### What problem does this PR solve? Problem Summary: Cloud loads previously excluded the memtable-on-sink path. Allow eligible full-row DUPLICATE, AGGREGATE, UNIQUE MOR, and UNIQUE MOW loads to build memtables and segments on sink BEs, then forward segment and index files to the target BE for object storage upload and rowset commit. Retain the restrictions for partial updates, V1 indexes, and row binlog. Pass transaction expiration, storage vault, and cache policy through stream-open requests. Select CloudRowsetBuilder in LoadStreamWriter, collect forwarded packed-file mappings and file sizes, and snapshot temporary rowset metadata before asynchronous MOW bitmap calculation on the target BE. Honor skip_writing_empty_rowset_metadata for untouched tablets by setting the policy before initialization and skipping prepare/commit RPCs while preserving empty markers and statistics. Close cloud tablet writers with concurrency 10 after pre-close, collecting all tablet results even when some fail. Preserve local-mode close behavior and existing locking. Add coverage for DUP/AGG/MOR/MOW, packed files, Stream Load, Broker Load, async Group Commit internal writer profiles, duplicate-key checks, cold reads, failed load invisibility and retry, and partial-update loading results. Add BE coverage for bitmap snapshot failures, empty-rowset lifecycle, and bounded concurrent close with mixed results. Align regression output blocks with the file-forwarding suite. ### Release note Cloud loads can enable memtable-on-sink for eligible full-row DUPLICATE, AGGREGATE, and UNIQUE MOR/MOW tables. Sink BEs construct files and target BEs upload them and calculate MOW bitmaps. Empty tablets honor the existing metadata-skip configuration, and cloud tablet closes run with bounded concurrency. ### Check List (For Author) - Test: Static call-site review, clang-format 16 checks, build hygiene checks, and source git diff --check passed. BE unit tests and regression cases added or updated; no compilation, source generation, or runtime tests performed during these changes, as requested. Unused regression output blocks removed without changing retained results; existing output-file trailing separators preserved. - Behavior changed: Yes; eligible Cloud loads use file forwarding, preserve empty metadata skipping, and close tablet writers concurrently. - Does this need documentation: Yes; design documents excluded from this commit at user request.
mymeiyi
force-pushed
the
cloud-load-1
branch
from
September 22, 2026 08:01
7f64912 to
cc5cea4
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27846 ms |
Contributor
TPC-DS: Total hot run time: 153311 ms |
Contributor
ClickBench: Total hot run time: 23.89 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
No description provided.