github-actions[bot] commented on code in PR #66598:
URL: https://github.com/apache/doris/pull/66598#discussion_r3748104999
##########
be/src/cloud/cloud_cumulative_compaction.cpp:
##########
@@ -425,17 +425,23 @@ Status CloudCumulativeCompaction::modify_rowsets() {
std::sort(pre_rowsets.begin(), pre_rowsets.end(), Rowset::comparator);
auto pre_rowsets_delete_bitmap =
std::make_shared<DeleteBitmap>(_tablet->tablet_id());
std::map<std::string, int64_t> pre_rowset_to_versions;
+ std::unique_ptr<CloudTablet::PreRowsetDeleteBitmapStats>
pre_rowset_delete_bitmap_stats;
+ if (config::enable_remove_pre_rowsets_delete_bitmap_by_keys) {
Review Comment:
**[P2] Skip source-stat collection in v2-only mode**
With `delete_bitmap_store_write_version == 2`,
`cloud_update_delete_bitmap_without_lock()` returns before building or sending
a v1 request, but this default-on branch still makes
`agg_delete_bitmap_for_compaction()` scan every retained source bitmap, call
`getSizeInBytes()`, and materialize one tuple per key. For tablets with the
large delete-bitmap histories this PR targets, every cumulative compaction pays
that O(K) CPU/allocation and shared-lock cost for data that is immediately
discarded. Please gate stats collection on a v1-writing mode (or move the
existing v2 early exit ahead of this aggregation) and cover the v2-only path.
##########
cloud/src/meta-service/meta_service.cpp:
##########
@@ -3941,6 +3941,124 @@ 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) {
+ std::string pre_rowset_id;
+ for (size_t i = 0; i < request->rowset_ids_size(); ++i) {
+ if (request->rowset_ids(i) == pre_rowset_id) {
+ continue;
+ }
+ if (non_exist_rowset_ids.contains(request->rowset_ids(i))) {
+ LOG(INFO) << "skip remove pre rowsets delete bitmap,
rowset_id="
+ << request->rowset_ids(i) << " tablet_id=" <<
tablet_id
+ << " because the rowset does not exist";
+ continue;
+ }
+ pre_rowset_id = request->rowset_ids(i);
+ auto delete_bitmap_start =
+ meta_delete_bitmap_key({instance_id, tablet_id,
request->rowset_ids(i),
+
request->pre_rowset_agg_start_version(), 0});
+ auto delete_bitmap_end =
+ meta_delete_bitmap_key({instance_id, tablet_id,
request->rowset_ids(i),
+
request->pre_rowset_agg_end_version(), 0});
+ txn->remove(delete_bitmap_start, delete_bitmap_end);
+ LOG(INFO) << "remove pre rowsets delete bitmap by range,
tablet_id=" << tablet_id
+ << ", rowset=" << request->rowset_ids(i)
+ << ", start_version=" <<
request->pre_rowset_agg_start_version()
+ << ", end_version=" <<
request->pre_rowset_agg_end_version()
+ << ", start_key=" << hex(delete_bitmap_start)
+ << ", end_key=" << hex(delete_bitmap_end);
+ }
+ return true;
+ }
+
+ for (const auto& rowset_stats : request->pre_rowset_delete_bitmap_stats())
{
+ if (non_exist_rowset_ids.contains(rowset_stats.rowset_id())) {
+ LOG(INFO) << "skip remove pre rowsets delete bitmap, rowset_id="
+ << rowset_stats.rowset_id() << " tablet_id=" << tablet_id
+ << " because the rowset does not exist";
+ continue;
+ }
+ uint64_t delete_key_count = 0;
+ for (const auto& delete_bitmap_stat :
rowset_stats.delete_bitmap_stats()) {
+ auto delete_bitmap_key = meta_delete_bitmap_key(
Review Comment:
**[P1] Keep cleanup stats inside the source range**
The current BE builder emits only versions below
`pre_rowset_agg_end_version`, but this endpoint acts destructively on the
protobuf without enforcing that cross-field invariant. A fully populated stats
list can include the end version alongside the real source entries. The handler
first writes the replacement bitmap there, then this loop clears every named
key; the end-version clear wins over the prior put while the actual source
entries are cleared too, so the successful transaction can leave neither copy.
This is distinct from missing optional fields because every field can be
present and well-typed. Please reject any stat outside
`[pre_rowset_agg_start_version, pre_rowset_agg_end_version)` before queuing a
clear, and add a negative case that includes the end-version aggregate.
--
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]
