github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4345907080
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,7 +1044,17 @@ int InstanceChecker::do_inverted_check() {
// so we choose to skip here.
TEST_SYNC_POINT_RETURN_WITH_VALUE("InstanceChecker::do_inverted_check",
(int)0);
- for (auto& [_, accessor] : accessor_map_) {
+ // Several resources may be backed by the same storage location, e.g.
storage vaults on the same
+ // bucket and prefix with different credentials. List each location only
once, otherwise its
+ // objects would be scanned and their leaks counted repeatedly.
+ std::unordered_set<std::string> checked_uris;
+ for (auto& [resource_id, accessor] : accessor_map_) {
+ if (!checked_uris.insert(accessor->uri()).second) {
Review Comment:
[P2] Deduplicate using a lossless storage identity
`uri()` is normalized for display: `S3Accessor::init()` runs it through
`normalize_http_uri()`, which collapses repeated slashes, while `get_key()` and
the S3 list request preserve the original prefix. Two valid vaults on the same
endpoint/bucket with prefixes `a/b` and `a//b` therefore have equal URIs but
list different object keys. This set skips one vault entirely, so leaks under
the skipped prefix remain undetected and the check can return success. Use a
storage identity that preserves the actual prefix bytes rather than the
normalized URI; the shared-MockAccessor test does not exercise this collision.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -950,8 +944,9 @@ int InstanceChecker::do_inverted_check() {
if (tablet_rowsets_cache.rowset_ids.contains(rowset_id)) {
return 0;
} else {
- LOG(WARNING) << "rowset not exists, key=" << obj_key;
- return -1;
+ // Garbage data leak, same as a cache miss below
+ LOG(WARNING) << "rowset should be recycled, key=" << obj_key;
+ return 1;
Review Comment:
[P2] Publish the tablet cache only after metadata loading succeeds
A cache hit does not currently imply that the metadata lookup succeeded: the
miss path sets `tablet_rowsets_cache.tablet_id` and clears its rowsets before
`create_txn()` / `txn->get()`, then leaves that cache in place on failure. If
the first segment of a live tablet encounters an FDB timeout, enumeration
continues and its next segment hits the empty cache here. The new `return 1`
counts that live file as a leak, increments `num_inverted_check_failed`, and
makes the final result `1` override the recorded `-1`. Mark the cache valid
only after a complete successful load, or invalidate it on every load failure,
so incomplete metadata cannot establish a leak.
--
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]
