github-actions[bot] commented on code in PR #68365:
URL: https://github.com/apache/doris/pull/68365#discussion_r4069167794
##########
be/src/cloud/cloud_rowset_builder.cpp:
##########
@@ -249,6 +249,15 @@ Status CloudRowsetBuilder::commit_rowset(const
std::string& job_id, int64_t tabl
return _engine.meta_mgr().commit_rowset(*rowset_meta(), job_id, table_id);
}
+Status CloudRowsetBuilder::commit_txn() {
+ DCHECK(is_data_builder());
+ RETURN_IF_ERROR(commit_rowset("", _tablet->table_id()));
Review Comment:
[P1] Preserve the cloud empty-rowset skip path here. V2 close sends every
tablet in a touched partition, so `IndexStream::close()` creates and
initializes writers even for untouched buckets; initialization has already
issued `PREPARE_ROWSET`, and this method now always issues `COMMIT_ROWSET`.
That bypasses the default-true `skip_writing_empty_rowset_metadata` behavior in
`CloudDeltaWriter::_commit_empty_rowset()`, which skips both RPCs and records
only the lightweight txn marker. Sparse loads will therefore create full rowset
metadata for every untouched bucket. Detect the zero-segment case before
builder initialization and retain the established skip/marker lifecycle.
##########
be/src/exec/pipeline/pipeline_fragment_context.cpp:
##########
@@ -1305,7 +1305,7 @@ Status
PipelineFragmentContext::_create_data_sink(ObjectPool* pool, const TDataS
int child_node_id = pipeline->operators().back()->node_id();
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)) {
Review 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.
##########
be/src/load/channel/load_stream_writer.cpp:
##########
@@ -317,9 +352,7 @@ Status LoadStreamWriter::close() {
RETURN_IF_ERROR(_pre_close());
}
RETURN_IF_ERROR(_rowset_builder->wait_calc_delete_bitmap());
- // 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());
Review 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.
##########
regression-test/data/cloud_p0/test_cloud_memtable_on_sink.out:
##########
@@ -0,0 +1,278 @@
+-- This file is automatically generated. You should know what you did if you
want to edit this
+-- !s3_rows_false --
+20 1133 2266
+
+-- !s3_index_false --
+31 62
+50 100
+50 100
+57 114
+
+-- !s3_packed_meta_false --
+6 20 2 1 1 0 0,1,2,3,4,5
+
+-- !s3_rows_true --
Review Comment:
[P2] Regenerate this expected output from the checked-in suite, or restore
the missing test branch. The current Groovy test goes from
`s3_packed_meta_false` to `stream_rows` and never emits
`s3_rows_true`/`s3_index_true`/`s3_packed_meta_true`; the same file also
contains unmatched failed-upload/layout and full `*_true` model blocks.
`OutputBlocksIterator.hasNextTagBlock()` silently drains unmatched blocks while
looking for the next requested tag, so this stale file can still pass while
claiming coverage that never ran.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
