github-actions[bot] commented on code in PR #66598:
URL: https://github.com/apache/doris/pull/66598#discussion_r3748733769
##########
cloud/src/meta-service/meta_service.cpp:
##########
@@ -3941,6 +3941,126 @@ 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())
{
Review Comment:
**[P1] Bind every cleanup stat to its replacement**
The stats list is independent of the aggregate-output arrays, so a fully
populated request can name `(rowset B, segment 0, version V)` while only
writing an aggregate for rowset A—or provide no output tuples at all. The
latter is exactly the shape used by `RemovePreDeleteBitmapBatchesEachBlobKey`:
this loop removes the only source bitmap and returns `OK`. With a real bitmap
that drops delete marks and can expose deleted rows. This is distinct from
missing optional fields and out-of-range versions. Before any transaction can
commit, validate the aligned output shape and require each destructive
`(rowset_id, segment_id)` to bind to an eligible output at
`pre_rowset_agg_end_version`; omit or explicitly handle empty-source stats, and
add stats-only plus A/B negative tests.
##########
cloud/src/meta-service/meta_service.cpp:
##########
@@ -3941,6 +3941,126 @@ 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()) {
+ DCHECK(delete_bitmap_stat.version() >=
request->pre_rowset_agg_start_version() &&
+ delete_bitmap_stat.version() <
request->pre_rowset_agg_end_version());
+ auto delete_bitmap_key = meta_delete_bitmap_key(
+ {instance_id, tablet_id, rowset_stats.rowset_id(),
delete_bitmap_stat.version(),
+ delete_bitmap_stat.segment_id()});
+ for (const auto& remove_key :
+ blob_remove_keys(delete_bitmap_key,
delete_bitmap_stat.delete_bitmap_size())) {
Review Comment:
**[P1] Bound the claimed blob work before expanding keys**
`delete_bitmap_size` is a caller-supplied `uint64`, and `blob_remove_keys()`
materializes `ceil(size / 90000) + 1` strings before this batching loop can
commit anything. A fully populated in-range stat with `UINT64_MAX` therefore
attempts to reserve roughly 205 trillion keys and can exhaust memory or throw
out of the RPC handler. Preflight before any writes: reject per-stat
sizes/chunk counts above the storable blob and suffix limits, reject
duplicates, and enforce an overflow-safe cumulative generated-key budget; then
generate keys incrementally. Add oversized and repeated-entry negative tests
that verify zero mutation.
--
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]
