github-actions[bot] commented on code in PR #66598:
URL: https://github.com/apache/doris/pull/66598#discussion_r3780790972
##########
be/src/cloud/cloud_meta_mgr.cpp:
##########
@@ -2246,6 +2248,20 @@ Status
CloudMetaMgr::cloud_update_delete_bitmap_without_lock(
req.set_pre_rowset_agg_start_version(pre_rowset_agg_start_version);
req.set_pre_rowset_agg_end_version(pre_rowset_agg_end_version);
}
+ if (pre_rowset_delete_bitmap_stats != nullptr) {
Review Comment:
**[P2] Bound the source-stat request while collecting it**
`agg_delete_bitmap_for_compaction()` retains one tuple for every source
`(rowset, segment, version)` key, and this block then copies that whole
inventory into protobuf before sending one RPC. The cumulative-compaction input
limit does not bound all older retained pre-rowsets, and there is no tuple,
allocation, or serialized-byte budget, so a valid large history can create an
unbounded extra memory peak or exceed the RPC body limit before Meta Service's
transaction batching starts. Please enforce an overflow-safe budget during
collection and, when exceeded, discard the entire stats field so the existing
range-cleanup path remains correct (or introduce bounded idempotent RPC
batches); add a low-budget test that proves the request is absent rather than
truncated.
##########
cloud/src/meta-service/meta_service.cpp:
##########
@@ -3803,6 +3803,78 @@ struct UpdateDeleteBitmapTxnStats {
size_t total_txn_count = 0;
};
+static bool check_delete_bitmap_point_delete(MetaServiceCode& code,
std::string& msg,
+ std::unique_ptr<Transaction>& txn,
+ const std::string& key, const
std::string& val,
+ const UpdateDeleteBitmapRequest*
request,
+ bool& point_delete, bool&
delete_bitmap_exists,
+ std::optional<size_t>&
max_blob_sequence) {
+ point_delete = config::enable_remove_pre_rowsets_delete_bitmap_by_keys &&
+ request->lock_id() ==
COMPACTION_WITHOUT_LOCK_DELETE_BITMAP_LOCK_ID;
+ if (!point_delete) {
+ return true;
+ }
+ std::string end_key {key};
+ encode_int64(INT64_MAX, &end_key);
+ RangeGetOptions opts;
+ opts.batch_limit = 1;
+ opts.reverse = true;
+ std::unique_ptr<RangeGetIterator> it;
+ auto err = txn->get(key, end_key, &it, opts);
Review Comment:
**[P2] Bound destination probes by transaction age**
This adds a synchronous reverse range read for every aggregate output, on
top of the existing per-output rowset lookup, while the transaction is split
only by affected bytes. A request with many small outputs can therefore perform
thousands of serial reads without reaching the byte threshold and cross
[FoundationDB's usual five-second transaction
lifetime](https://apple.github.io/foundationdb/known-limitations.html);
`MetaServiceProxy` then replays the handler from output zero without preserving
progress, and BE does not retry the surfaced `KV_TXN_TOO_OLD` after server
retries are exhausted. Please add a probe-count/time boundary or a
conflict-safe batched lookup, preserving the replacement-before-cleanup
ordering, and test a later probe aging out with forward progress.
##########
cloud/src/meta-service/meta_service.cpp:
##########
@@ -3956,6 +4052,136 @@ void _write_delete_bitmap_kvs(MetaServiceCode& code,
std::string& msg, std::stri
<< " key_size: " << key.size() << " value_size: " << val.size();
}
+static bool commit_pre_rowset_delete_bitmap_removal(
+ MetaServiceCode& code, std::string& msg, std::stringstream& ss,
+ const std::shared_ptr<TxnKv>& txn_kv, std::unique_ptr<Transaction>&
txn, KVStats& stats,
+ UpdateDeleteBitmapTxnStats& txn_stats, int64_t tablet_id, const
std::string& rowset_id) {
+ auto txn_size = txn->approximate_bytes();
+ LOG(INFO) << "commit delete bitmap point deletes before transaction size
exceeds limit, "
+ "tablet_id="
+ << tablet_id << ", rowset=" << rowset_id << ", txn_size=" <<
txn_size;
+ auto err = txn->commit();
+ TEST_SYNC_POINT_CALLBACK("update_delete_bitmap:remove_pre_rowsets:commit",
txn_size);
+ txn_stats.total_txn_put_keys += txn->num_put_keys();
+ txn_stats.total_txn_put_bytes += txn->put_bytes();
+ txn_stats.total_txn_size += txn_size;
+ txn_stats.total_txn_count++;
+ if (err != TxnErrorCode::TXN_OK) {
+ code = cast_as<ErrCategory::COMMIT>(err);
+ ss << "failed to remove pre rowsets delete bitmap, err=" << err
+ << " tablet_id=" << tablet_id << " rowset_id=" << rowset_id << "
txn_size=" << txn_size;
+ msg = ss.str();
+ g_bvar_update_delete_bitmap_fail_counter << 1;
+ return false;
+ }
+ stats.get_bytes += txn->get_bytes();
+ stats.put_bytes += txn->put_bytes();
+ stats.del_bytes += txn->delete_bytes();
+ stats.get_counter += txn->num_get_keys();
+ stats.put_counter += txn->num_put_keys();
+ stats.del_counter += txn->num_del_keys();
+ txn_stats.current_key_count = 0;
+ txn_stats.current_value_count = 0;
+ err = txn_kv->create_txn(&txn);
+ if (err != TxnErrorCode::TXN_OK) {
+ code = cast_as<ErrCategory::CREATE>(err);
+ msg = "failed to init txn when removing pre rowsets delete bitmap";
+ return false;
+ }
+ return true;
+}
+
+static bool remove_pre_rowset_delete_bitmap(
+ MetaServiceCode& code, std::string& msg, std::stringstream& ss,
+ const std::shared_ptr<TxnKv>& txn_kv, std::unique_ptr<Transaction>&
txn, KVStats& stats,
+ const UpdateDeleteBitmapRequest* request, const std::string&
instance_id,
+ const std::set<std::string>& non_exist_rowset_ids,
UpdateDeleteBitmapTxnStats& txn_stats) {
+ if (!request->has_pre_rowset_agg_start_version() ||
+ !request->has_pre_rowset_agg_end_version() ||
+ request->pre_rowset_agg_start_version() >=
request->pre_rowset_agg_end_version()) {
+ return true;
+ }
+
+ auto tablet_id = request->tablet_id();
+ if (request->pre_rowset_delete_bitmap_stats_size() == 0) {
Review Comment:
**[P2] Apply the MS switch to source point cleanup**
The Meta-Service flag is documented as controlling pre-rowset removal by
key, but it is checked only when replacing aggregate output keys. With
`BE=true, MS=false` in write modes 1 or 3, the request still carries stats and
this condition unconditionally selects exact source-key cleanup, including the
retained-tail behavior that disabling the MS flag should avoid. Please make
MS=false treat the stats as absent and take the existing range branch, or
split/rename the controls so their narrower scopes are explicit; add a
stats-bearing MS=false test, including an underestimated multi-fragment source.
--
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]
