github-actions[bot] commented on code in PR #68322:
URL: https://github.com/apache/doris/pull/68322#discussion_r4081619224
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3710,7 +3707,37 @@ int InstanceRecycler::recycle_tablets(int64_t table_id,
int64_t index_id,
}
}
}
- if (is_multi_version) {
+ if (should_recycle_versioned_keys()) {
+ // Remove tablet indexes in the same transaction as tablet
metadata.
+ for (const auto& tablet_info : tablets_info) {
+ std::string versioned_idx_key =
+ versioned::tablet_index_key({instance_id_,
tablet_info.tablet_id});
+ std::string tablet_index_val;
+ TxnErrorCode err = txn->get(versioned_idx_key,
&tablet_index_val);
Review Comment:
[P1] Batch the page-level tablet-index reads
A FoundationDB range page can contain hundreds to roughly 1,800 small
detached tablet records, and this new loop calls `Transaction::get` once per
tablet. That implementation creates a future and immediately waits for it, so
all point reads are serialized after the transaction establishes its read
version. At ordinary millisecond-scale latency the loop can exceed
FoundationDB's supported five-second read/write transaction lifetime before
commit. The failed page retains its tablet metadata and outer marker, so each
recycler invocation reconstructs the same page and a large dropped
index/partition never converges. Please fetch these keys with the existing
non-snapshot `batch_get` (preserving the conflict set), or otherwise bound
elapsed work per transaction; the one-tablet MemTxn test cannot exercise this
production path.
##########
cloud/src/recycler/recycler.cpp:
##########
@@ -3710,7 +3707,37 @@ int InstanceRecycler::recycle_tablets(int64_t table_id,
int64_t index_id,
}
}
}
- if (is_multi_version) {
+ if (should_recycle_versioned_keys()) {
Review Comment:
[P2] Keep metadata cleanup active for abandoned WRITE_ONLY creates
In `MULTI_VERSION_WRITE_ONLY`, tablet creation already writes versioned
metadata and stats, but `should_recycle_versioned_keys()` is false while the
snapshot switch is absent/disabled. If FE crashes after creating tablets but
before committing its PREPARED index/partition, the expired marker reaches this
branch: the old `is_multi_version` condition removed those metadata/stats,
whereas this condition leaves them behind and the enclosing recycler still
deletes the only marker. The versioned forward index could already survive this
state before the patch, so retaining metadata now turns that dangling identity
into a resolvable stale tablet, with no later tablet-orphan scan to reclaim it.
Please keep metadata cleanup eligible for every non-disabled state (or retain a
durable reconciliation marker), and add a PREPARED + WRITE_ONLY +
snapshot-disabled 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]
