github-actions[bot] commented on code in PR #67989:
URL: https://github.com/apache/doris/pull/67989#discussion_r4035570530
##########
gensrc/proto/cloud.proto:
##########
@@ -1085,6 +1085,9 @@ message CommitTxnRequest {
optional string request_ip = 12;
optional int64 commit_tso = 13;
repeated TableStreamUpdatePB table_stream_updates = 14;
+ // Distinct partitions reported by BE, across all sub-transactions. Omit
if unknown.
+ // MS omits version results on mismatch so FE invalidates its version
caches.
+ optional int64 num_partitions = 16;
Review Comment:
[P1] Cover the BE-direct stream-load commit producer
When `enable_stream_load_commit_txn_on_be` is enabled, eligible stream loads
build this same request in `CloudMetaMgr::commit_txn`, enable lazy commit, and
never set `num_partitions`. MS therefore skips this check for that supported
path, then BE forwards a possibly partial response to `afterCommitTxnResp`,
which updates only the returned partitions. The stale lazy-retry interleaving
can still leave FE caches behind for BE-direct loads. Please populate an
equivalent complete partition count/invalidation signal there and cover that
caller.
##########
cloud/src/meta-service/txn_lazy_committer.cpp:
##########
@@ -918,7 +918,7 @@ std::pair<MetaServiceCode, std::string>
TxnLazyCommitTask::commit_partition(
LOG(INFO) << "remove tmp_rowset_key=" << hex(tmp_rowset_key) << "
txn_id=" << txn_id_;
}
- TEST_SYNC_POINT_CALLBACK("TxnLazyCommitter::commit");
+ TEST_SYNC_POINT_CALLBACK("TxnLazyCommitter::commit", &partition_id);
Review Comment:
[P1] Protect the lazy publisher from tmp-value replacement too
The new immediate-path read does not cover this lazy path.
`TxnLazyCommitTask::commit` passes protobufs decoded by an earlier range scan
into `convert_tmp_rowsets`; that function reads the current tmp key but
discards those bytes and publishes the captured protobuf. An
`update_tmp_rowset` committed before that read therefore publishes stale
segments/stats, and one committed after conversion but before this blind
cleanup is deleted without being published. Please consume/compare the
in-transaction value and preserve that dependency through cleanup, with a
replacement-race test.
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -3050,6 +3099,7 @@ void MetaServiceImpl::commit_txn_eventually(
// txn set visible for fe callback
txn_info.set_status(TxnStatusPB::TXN_STATUS_VISIBLE);
response->mutable_txn_info()->CopyFrom(txn_info);
+ check_commit_txn_partition_count(request, response);
Review Comment:
[P2] Build lazy response stats from the rowsets actually published
The lazy task rescans tmp rowsets after this handler's outer scan and may
legitimately publish a replacement written by a late `update_tmp_rowset`. After
task success, however, `commit_txn_eventually` still calculates
`TableStatsPB.updated_row_count` from the old `tmp_rowsets_meta` captured
before the first phase. Because the partition set is unchanged, this count
check passes and FE journals the stale row-change total to `AnalysisManager`.
Please return accounting from the protobufs accepted by the lazy task (or mark
it unknown) and cover a replacement between the two scans.
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -1800,6 +1800,26 @@ std::pair<MetaServiceCode, std::string>
get_partition_versions(
return {MetaServiceCode::OK, ""};
}
+static void check_commit_txn_partition_count(const CommitTxnRequest* request,
+ CommitTxnResponse* response) {
+ int64_t num_partitions = response->partition_ids_size();
+ if (!request->has_num_partitions() || request->num_partitions() ==
num_partitions) {
+ return;
+ }
+ LOG(WARNING) << "commit txn partition count mismatch, txn_id=" <<
request->txn_id()
+ << " table_ids="
+ << fmt::format("[{}]",
fmt::join(response->txn_info().table_ids(), ", "))
+ << " expected_partitions=" << request->num_partitions()
+ << " actual_partitions=" << num_partitions;
+ // A retry may scan only partitions left by a concurrent lazy committer.
+ // Empty both version lists and table stats to trigger FE cache
invalidation.
+ response->clear_table_ids();
+ response->clear_partition_ids();
+ response->clear_versions();
+ response->clear_table_stats();
Review Comment:
[P2] Do not drop row-change accounting to signal invalidation
`TableStatsPB` also carries `updated_row_count`, the only cloud-load input
to `AnalysisManager.updateUpdatedRows`. In the supported mismatch caused by an
entirely filtered partition, the other partitions' published-row accounting is
valid, but this clears it and can suppress update-rate auto analysis. Please
separate the untrusted table-version hint from the mismatch/invalidation signal
(or otherwise preserve safe row-change accounting) and test a mismatch with
nonzero published rows.
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -3050,6 +3099,7 @@ void MetaServiceImpl::commit_txn_eventually(
// txn set visible for fe callback
txn_info.set_status(TxnStatusPB::TXN_STATUS_VISIBLE);
response->mutable_txn_info()->CopyFrom(txn_info);
+ check_commit_txn_partition_count(request, response);
Review Comment:
[P1] Preserve a refresh trigger for incomplete lazy mismatches
This check can clear every version hint after the first lazy phase has
advanced `table_version`, even when `task->wait()` failed and
`is_lazy_commit_incomplete` remains true. FE then explicitly skips
invalidation. The sync daemon can observe that new table version while
partitions are still pending, fetch their old versions with
`waitForPendingTxns=false`, and cache both; background lazy recovery has no FE
callback and does not advance the table version again. With the default
non-expiring caches, those partitions can therefore remain stale indefinitely.
Please carry an explicit revalidation signal through incomplete recovery (or
make synchronization pending-aware) and add the combined
mismatch/failure/recovery cache test.
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -3660,6 +3710,7 @@ void MetaServiceImpl::commit_txn_with_sub_txn(const
CommitTxnRequest* request,
}
response->mutable_txn_info()->CopyFrom(txn_info);
+ check_commit_txn_partition_count(request, response);
Review Comment:
[P1] Apply tmp-rowset value validation to transaction loads
`commit_txn_with_sub_txn` bypasses the new immediate-path validation: it
scans each sub-transaction before creating the write transaction, publishes
those captured protobufs, and later blindly deletes their tmp keys. A
late/retried `update_tmp_rowset` can commit after the scan without conflicting,
so its merged segments/statistics are lost. Please read and compare/rebuild
every sub-transaction tmp value in the committing transaction and add a
concurrent replacement test.
--
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]
