github-actions[bot] commented on code in PR #69264:
URL: https://github.com/apache/doris/pull/69264#discussion_r4311131925
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1249,6 +1249,135 @@ int InstanceChecker::collect_tablet_rowsets(
return 0;
}
+std::pair<MetaServiceCode, std::string> check_delete_bitmap_orphan(
+ const std::shared_ptr<TxnKv>& txn_kv, const std::string& instance_id,
int64_t tablet_id,
+ int64_t max_scan_keys, DeleteBitmapOrphanCheckResult* result,
+ const std::function<bool()>& should_cancel) {
+ *result = {};
+ auto check_cancelled = [&] {
+ result->cancelled = should_cancel();
+ return result->cancelled;
+ };
+ std::unique_ptr<Transaction> txn;
+ auto err = txn_kv->create_txn(&txn);
+ if (err != TxnErrorCode::TXN_OK) {
+ return {cast_as<ErrCategory::CREATE>(err), "failed to create check
transaction"};
+ }
+ std::string instance_value;
+ err = txn->get(instance_key({instance_id}), &instance_value);
Review Comment:
[P2] Check cancellation around the initial instance lookup
`should_cancel` is first invoked only after the first bitmap range read. An
already-cancelled request therefore still reads the instance; when it is
absent, the handler returns HTTP 400 before observing cancellation.
Cancellation arriving during that lookup similarly returns `INVALID_ARGUMENT`
instead of `OK` with `result.cancelled=true`. This directly contradicts both
added tests `delete_bitmap_orphan_http_cancelled_before_scan` and
`delete_bitmap_orphan_check_cancelled_during_instance_read`; an
existing-instance request can also start a bitmap read after cancellation.
Check cancellation before initial I/O and immediately after the instance
lookup, before interpreting its result, so these paths return the intended
cancelled result.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1249,6 +1249,135 @@ int InstanceChecker::collect_tablet_rowsets(
return 0;
}
+std::pair<MetaServiceCode, std::string> check_delete_bitmap_orphan(
+ const std::shared_ptr<TxnKv>& txn_kv, const std::string& instance_id,
int64_t tablet_id,
+ int64_t max_scan_keys, DeleteBitmapOrphanCheckResult* result,
+ const std::function<bool()>& should_cancel) {
+ *result = {};
+ auto check_cancelled = [&] {
+ result->cancelled = should_cancel();
+ return result->cancelled;
+ };
+ std::unique_ptr<Transaction> txn;
+ auto err = txn_kv->create_txn(&txn);
+ if (err != TxnErrorCode::TXN_OK) {
+ return {cast_as<ErrCategory::CREATE>(err), "failed to create check
transaction"};
+ }
+ std::string instance_value;
+ err = txn->get(instance_key({instance_id}), &instance_value);
+ if (err == TxnErrorCode::TXN_KEY_NOT_FOUND) {
+ return {MetaServiceCode::INVALID_ARGUMENT, "instance not found"};
+ }
+ if (err != TxnErrorCode::TXN_OK) {
+ return {cast_as<ErrCategory::READ>(err), "failed to read instance"};
+ }
+
+ auto begin = meta_delete_bitmap_key({instance_id, tablet_id, "", 0, 0});
+ auto end = meta_delete_bitmap_key(
+ {instance_id, tablet_id == 0 ? INT64_MAX : tablet_id + 1, "", 0,
0});
+ RangeGetOptions options;
+ options.reverse = true;
+ options.snapshot = true;
+ int64_t cached_tablet_id = -1;
+ std::unordered_set<std::string> rowsets;
+ while (true) {
+ if (max_scan_keys > 0 && result->scanned_kvs >= max_scan_keys) {
+ return {MetaServiceCode::OK, ""};
+ }
+ // Small pages bound bitmap value traffic before an early return. No
next-page prefetch.
+ options.batch_limit = max_scan_keys > 0
+ ? std::min<int64_t>(128, max_scan_keys -
result->scanned_kvs)
+ : 128;
+ std::unique_ptr<RangeGetIterator> it;
+ err = txn->get(begin, end, &it, options);
+ if (check_cancelled()) {
+ return {MetaServiceCode::OK, ""};
+ }
+ if (err != TxnErrorCode::TXN_OK) {
+ return {cast_as<ErrCategory::READ>(err), "failed to scan v1 delete
bitmaps"};
+ }
+ // The cache is reused across pages, each with a new transaction.
Track whether the
+ // current tablet's cache was refreshed at this transaction's read
version, so a cache
+ // miss is rechecked for newly published rowsets before reporting a
suspected orphan.
+ bool rowsets_cache_refreshed_in_txn = false;
+ while (!check_cancelled() && it->has_next()) {
+ auto [key, value] = it->next();
+ ++result->scanned_kvs;
+ auto encoded_key = key.substr(1);
+ std::vector<std::tuple<std::variant<int64_t, std::string>, int,
int>> fields;
+ // Legacy keys have seven fields; blob keys have an additional
encoded suffix.
+ if (decode_key(&encoded_key, &fields) != 0 ||
+ (fields.size() != 7 && fields.size() != 8)) {
+ return {MetaServiceCode::UNDEFINED_ERR,
+ fmt::format("malformed delete bitmap key: {}",
hex(key))};
+ }
+ const auto* current_tablet_id =
std::get_if<int64_t>(&std::get<0>(fields[3]));
+ const auto* rowset_id =
std::get_if<std::string>(&std::get<0>(fields[4]));
+ const auto* version =
std::get_if<int64_t>(&std::get<0>(fields[5]));
+ const auto* segment_id =
std::get_if<int64_t>(&std::get<0>(fields[6]));
+ if (current_tablet_id == nullptr || rowset_id == nullptr ||
version == nullptr ||
+ segment_id == nullptr) {
+ return {MetaServiceCode::UNDEFINED_ERR,
+ fmt::format("invalid delete bitmap key fields: {}",
hex(key))};
+ }
+
+ if (cached_tablet_id != *current_tablet_id ||
+ (!rowsets.contains(*rowset_id) &&
!rowsets_cache_refreshed_in_txn)) {
+ rowsets.clear();
+ FullRangeGetOptions rowset_options;
+ rowset_options.snapshot = true;
+ rowset_options.batch_limit = 128;
+ auto rowset_it = txn->full_range_get(
Review Comment:
[P2] Renew transactions within the nested rowset scans
`txn->full_range_get()` pins `opts.txn=this`, so all its 128-row pages keep
using the bitmap transaction; that transaction is replaced only after the whole
bitmap page is processed. A page containing one bitmap from each of 128 tablets
with 1,900 formal rowsets requires at least 1,920 sequential rowset reads on
one snapshot. At 3 ms per read this exceeds [FoundationDB's five-second read
window](https://apple.github.io/foundationdb/known-limitations.html#long-running-transactions):
a later rowset page returns `TXN_TOO_OLD`, and this endpoint errors out before
reaching lower keys. These rowset counts are below the configured 2,000-version
limit. Bound transaction lifetime within the nested scans and revalidate after
renewal, rather than relying solely on the outer bitmap page boundary.
--
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]
