You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "matanper (via GitHub)" <gi...@apache.org> on 2026/10/19 13:00:27 UTC
[PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
matanper opened a new pull request, #69290:
URL: https://github.com/apache/doris/pull/69290
### What problem does this PR solve?
Issue Number: close #69289
Related PR: #69278
Problem Summary:
#### What problem was fixed, and how
The cloud checker's inverted check (`enable_inverted_check = true`) is the leak detector. It lists the objects of each storage location and reports objects that no longer belong to any rowset, i.e. data the recycler failed to delete.
1. **It never ran for instances with more than one storage vault.** `InstanceChecker::do_inverted_check` started with:
```cpp
if (accessor_map_.size() > 1) {
LOG(INFO) << "currently not support inverted check for multi accessor. instance_id=" << instance_id_;
return 0;
}
```
The checker creates one accessor per storage vault, plus the built-in one. As soon as an instance has a second vault, nothing is inverted-checked for any vault and `num_inverted_check_failed` stays 0. The guard came with #33280 without a stated reason. The loop below it already iterates over all accessors, and nothing in the check depends on a single accessor: objects are matched to rowset meta by tablet id and rowset id only.
- Fix: remove the guard. List each storage location (accessor `uri()`, i.e. `endpoint/bucket/prefix` or `fs_name/prefix`) once, so several resources backed by the same location (e.g. vaults on one bucket and prefix with different credentials) are not scanned and counted repeatedly.
2. **A leak was reported or not depending on listing order.** In `check_segment_file`, a segment whose rowset no longer exists returned `1` (leak) only on a tablet-cache miss. On a cache hit (the previous object belonged to the same tablet), the same situation returned `-1` ("rowset not exists"). That is counted as a check error, not a leak, so a leaked segment listed after a live rowset of its tablet was never counted in `num_file_leak`.
- Fix: return `1` with the same "rowset should be recycled" log on the cache-hit path.
#### Which behaviors were modified
| Before | Now | Impact |
|---|---|---|
| Inverted check skipped on instances with more than one accessor (every multi-vault instance). | Runs for every distinct storage location of the instance. | Leaks on multi-vault instances start being reported (`num_inverted_check_failed`). The check lists every vault's `data/` directory, so its cost grows with the number of vaults. It is still gated by `enable_inverted_check` and `check_object_interval_seconds`. |
| A leaked segment after a live rowset of the same tablet: check error (`-1`). | Leak (`1`). | `num_file_leak` counts it, and the check result is `1` (leak) instead of `-1` (error). |
#### What features were added, and why
None besides the fix. Test-only: `MockAccessor` takes an optional uri, so tests can model distinct storage locations.
#### Which code was refactored, and why
None.
#### Which functions were optimized
None.
#### Relation to #69278
#69278 makes the recycler and checker understand the `path_version = 1` object layout per vault. This PR is independent and based on master. If both are merged, the checker inverted-checks v1 vaults too, which the guard currently prevents. The two touch neighbouring lines of `do_inverted_check`; the conflict is trivial.
### Release note
The cloud checker's inverted check (`enable_inverted_check`) now runs for instances with multiple storage vaults (it was silently skipped before), and leaked segment files are counted consistently.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
**Unit tests (ASAN, `run-cloud-ut.sh`):**
- `recycler_test:CheckerTest.*`: 42/42 pass, including the new tests:
- `CheckerTest.inverted_check_multi_vault`: two vaults, each object checked (`num_inverted_scanned = 60`). One leak per vault, one of them on the cache-hit path, gives result 1 and `num_inverted_check_failed = 2`.
- `CheckerTest.inverted_check_same_location_once`: two resources on one location are listed once (11 scanned, 1 leak).
- `CheckerTest.inverted_check_leak_after_live_rowset`: a leak listed after a live rowset of its tablet is counted as a leak.
- Full `recycler_test`: 204/205. The only failure is `RecyclerBenchmarkTest.RecycleRowsets`, which needs a live FoundationDB (it builds `FdbTxnKv` from `fdb.cluster`) and fails the same way on master.
- The existing tests that register a mocked HDFS vault next to obj_info "1" still pass.
**Manual test:**
- Setup: cloud-mode cluster with this branch's `doris_cloud` (meta-service + recycler, `enable_checker = true`, `enable_inverted_check = true`, `check_object_interval_seconds = 60`). Two S3 storage vaults plus the built-in vault, one table per vault, 3 loads each, `enable_packed_file = false` so segments are plain objects.
- Before (master): every checker round logs `currently not support inverted check for multi accessor` and checks nothing.
- After, with live data: `inverted check instance objects finished ... num_scanned=10 num_file_leak=0`. That covers all 5 + 5 objects of the two vaults, and the guard message is gone.
- After putting `data/<tablet_id>/zzzleak_0.dat` into each vault (both tablets also have live rowsets): `num_scanned=12 num_file_leak=2`, with `rowset should be recycled, key=data/<tablet_id>/zzzleak_0.dat` for both, and no `rowset not exists` lines.
- Behavior changed:
- [ ] No.
- [x] Yes. See "Which behaviors were modified" above.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg: https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "matanper (via GitHub)" <gi...@apache.org>.
matanper commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4376318440
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,14 +1052,32 @@ 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. Compare the location as
+ // configured, not the normalized `uri()`: prefixes "a/b" and "a//b" share a uri but hold
+ // different objects, and skipping one would hide its leaks.
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
Review Comment:
Fixed in 22b5e5a254a, root made alias-safe in a7ce4faabe7. Each accessor now has a `StorageRoot` (the S3 bucket, empty for HDFS, plus the prefix without empty path components). When another vault's root is inside the listed `data/` of the vault being scanned (e.g. `root/data` inside `root`), the parent skips that `data/.../data/` subtree; those objects are checked once, when the child vault is listed. Test: `InvertedCheckTest.nested_vault`.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -946,16 +940,19 @@ int InstanceChecker::do_inverted_check() {
return -1;
}
- if (tablet_rowsets_cache.tablet_id == tablet_id) {
- if (tablet_rowsets_cache.rowset_ids.contains(rowset_id)) {
- return 0;
- } else {
- LOG(WARNING) << "rowset not exists, key=" << obj_key;
- return -1;
- }
+ if (tablet_rowsets_cache.tablet_id == tablet_id &&
Review Comment:
Fixed in 22b5e5a254a, root made alias-safe in a7ce4faabe7. The tablet cache now keeps `rowset_id -> resource_id`, and a segment is only covered by its rowset if the rowset's vault has the root it was listed from; otherwise it is a leak (a cache hit that fails the check re-reads the meta first, as for a miss). Ownership compares roots, not listing locations, and the root is built to err toward "same": it is the bucket (empty for HDFS) plus the prefix without empty path components, with no endpoint/fs_name since those have aliases (http/https, internal/public endpoints; host/FQDN/IP/nameservice). So a different root surely means different paths, and vaults that may be the same storage (other credentials, an endpoint alias) accept each other's objects: an ambiguity can only hide a copy, never report live data. A resource with no accessor (an HDFS vault in a build without HDFS) is trusted. Test: `InvertedCheckTest.segment_in_another_vault_is_leak`.
##########
cloud/src/recycler/hdfs_accessor.cpp:
##########
@@ -325,11 +327,44 @@ class HdfsListIterator final : public ListIterator {
std::vector<DirEntries> level_dirs_;
};
-HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
- : StorageVaultAccessor(AccessorType::HDFS), info_(info), prefix_(info.prefix()) {
- if (!prefix_.empty() && prefix_[0] != '/') {
- prefix_.insert(prefix_.begin(), '/');
+namespace {
+
+// Either empty or starts with '/', e.g. "p1" and "/p1" are the same directory
+std::string accessor_prefix(const HdfsVaultInfo& info) {
+ std::string prefix = info.prefix();
+ if (!prefix.empty() && prefix[0] != '/') {
+ prefix.insert(prefix.begin(), '/');
}
+ return prefix;
+}
+
+// fs_name and prefix name the directory, but a logical nameservice in fs_name resolves through the
+// vault's hdfs_confs: two vaults with fs_name hdfs://ns and prefix /warehouse are different
+// clusters when their dfs.namenode.rpc-address.ns.* differ. Append the effective confs, sorted by
+// key, so equal locations mean equal configurations; differences that do not matter only cost a
+// second listing. '\n' separates the parts, so e.g. hdfs://ns/a + /b and hdfs://ns + /a/b differ.
+std::string accessor_location(const HdfsVaultInfo& info) {
+ // libhdfs applies builder confs so that the first value set for a key wins: keep that one
+ std::map<std::string, std::string> confs;
+ for (const auto& conf : info.build_conf().hdfs_confs()) {
+ confs.emplace(conf.key(), conf.value());
Review Comment:
Fixed in 22b5e5a254a: the location keeps all `hdfs_confs` in their configured order instead of picking a winner, so identical locations mean identical configurations under either precedence. A reordering now only costs an extra listing (the root, and so ownership, ignores confs). Test: `HdfsAccessorLocationTest.location`.
##########
cloud/src/recycler/s3_accessor.cpp:
##########
@@ -310,8 +310,12 @@ std::optional<S3Conf> S3Conf::from_obj_store_info(const ObjectStoreInfoPB& obj_i
return s3_conf;
}
+// The location is endpoint, bucket and prefix as configured; `uri_` normalizes them for display.
+// '\n' never occurs in an endpoint or a bucket name, so different confs never collide.
S3Accessor::S3Accessor(S3Conf conf)
- : StorageVaultAccessor(AccessorType::S3), conf_(std::move(conf)) {}
+ : StorageVaultAccessor(AccessorType::S3,
+ conf.endpoint + '\n' + conf.bucket + '\n' + conf.prefix),
Review Comment:
Fixed in 22b5e5a254a: the S3 location adds provider, ak, role_arn, external_id and the credential provider type (never the sk), so vaults that differ only in identity are listed separately. The root stays endpoint/bucket/prefix, so the duplicate listing does not turn into false leaks. Test: `S3AccessorLocationTest.location_includes_the_credential_identity`.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6342878873
# Cloud UT Coverage Report
Increment line coverage `100.00% (35/35)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/469f2566f1d2d9ffcdbd9a86fda5b5760af42653_469f2566f1d2d9ffcdbd9a86fda5b5760af42653_cloud/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/469f2566f1d2d9ffcdbd9a86fda5b5760af42653_469f2566f1d2d9ffcdbd9a86fda5b5760af42653_cloud/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 78.11% (2084/2668) |
| Line Coverage | 66.29% (38382/57902) |
| Region Coverage | 53.65% (35993/67087) |
| Branch Coverage | 57.21% (11601/20278) |
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6410049750
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6335640259
run buildall
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6267809376
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://doris.apache.org/community/how-to-contribute/review-and-ci).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4346909331
##########
cloud/src/recycler/checker.cpp:
##########
@@ -991,6 +990,11 @@ int InstanceChecker::do_inverted_check() {
}
}
}
+ if (stopped()) {
+ // The loop above stops early when the checker is stopped, the rowsets are incomplete
+ return -1;
Review Comment:
[P2] Stop object traversal when the checker is canceled
If shutdown or lease loss sets `stopped_` during a tablet load, this returns only from `check_segment_file` and leaves `tablet_rowsets_cache.tablet_id == 0`. The enclosing object loop keeps going; each remaining `.dat` for that same tablet then creates a new transaction and executes the first range read because `it == nullptr` makes the read loop run even when stopped. Previously the eagerly assigned tablet id let those subsequent objects reuse the cache, so this introduces a metadata read per remaining segment after cancellation. On a large tablet this can add thousands of FDB reads while `Checker::stop()` waits in `join()`, or after the worker has lost its lease. Propagate cancellation to the outer traversal before starting any further metadata work.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,7 +1053,19 @@ 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. Compare the location as
+ // configured, not the normalized `uri()`: prefixes "a/b" and "a//b" share a uri but hold
+ // different objects, and skipping one would hide its leaks.
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
+ if (!checked_locations.insert(accessor->location()).second) {
Review Comment:
[P2] Give HDFS vaults their actual storage location
For two HDFS vaults with different prefixes or namenodes, this condition still skips one: `HdfsAccessor` inherits `location()`, which returns `StorageVaultAccessor::uri_`, but its constructor initializes a separate, shadowing `HdfsAccessor::uri_` (`hdfs_accessor.h:83`, `hdfs_accessor.cpp:333`). Both accessors therefore return the empty string. Only the first vault is listed, so leaks in all other HDFS vaults remain invisible and the check can return success. This is distinct from the previously reported S3 normalized-prefix collision, which the new S3 override fixes. Provide an HDFS location implementation (or populate the base member) and cover distinct HDFS vaults.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6355353733
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4355361062
##########
cloud/src/recycler/hdfs_accessor.cpp:
##########
@@ -325,11 +325,24 @@ class HdfsListIterator final : public ListIterator {
std::vector<DirEntries> level_dirs_;
};
-HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
- : StorageVaultAccessor(AccessorType::HDFS), info_(info), prefix_(info.prefix()) {
- if (!prefix_.empty() && prefix_[0] != '/') {
- prefix_.insert(prefix_.begin(), '/');
+namespace {
+
+// Either empty or starts with '/', e.g. "p1" and "/p1" are the same directory
+std::string accessor_prefix(const HdfsVaultInfo& info) {
+ std::string prefix = info.prefix();
+ if (!prefix.empty() && prefix[0] != '/') {
+ prefix.insert(prefix.begin(), '/');
}
+ return prefix;
+}
+
+} // namespace
+
+HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
+ : StorageVaultAccessor(AccessorType::HDFS,
+ info.build_conf().fs_name() + accessor_prefix(info)),
Review Comment:
[P2] Include nameservice resolution in the HDFS location identity
With the supported enable_distinguish_hdfs_path=false setting, two HA vaults can both use fs_name=hdfs://ns and prefix=/warehouse while their per-vault dfs.namenode.rpc-address.ns.nn1/nn2 settings point to different clusters. CREATE preserves those settings, and HDFSBuilder applies them to separate forced-new connections, but both locations here become hdfs://ns/warehouse. The checker consequently skips one namespace and can return success despite leaks there. Include namespace-resolving configuration in the identity, or avoid deduplicating accessors whose configurations do not prove equivalence, and cover equal logical names with different address mappings. This is distinct from the fixed empty/shadowed URI issue: both locations are populated but resolve differently.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -950,12 +944,16 @@ 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] Revalidate a negative cache lookup before counting a leak
A successful tablet read can become stale while this scan runs. For example, after the first S3 page loads tablet T's existing rowsets into the cache, an ordinary load can upload and fully commit a new rowset of T whose segment sorts into a later page. The lazy listing then yields that live segment, but this branch never refreshes metadata: it now returns 1 and increments num_inverted_check_failed instead of the previous temporary error. The checker lease does not stop ingestion, and there is no file-age boundary here. Recheck a cached absence in a fresh transaction before declaring a leak, or establish a consistent age/snapshot boundary. This also needs an interleaving test; completing the initial read successfully does not prevent it.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6337645589
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "wyxxxcat (via GitHub)" <gi...@apache.org>.
wyxxxcat commented on PR #69290:
URL: https://github.com/apache/doris/pull/69290#issuecomment-6335649834
/review
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](cloud) Run checker inverted check for instances with multiple storage vaults [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #69290:
URL: https://github.com/apache/doris/pull/69290#discussion_r4374759088
##########
cloud/src/recycler/checker.cpp:
##########
@@ -1049,14 +1052,32 @@ 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. Compare the location as
+ // configured, not the normalized `uri()`: prefixes "a/b" and "a//b" share a uri but hold
+ // different objects, and skipping one would hide its leaks.
+ std::unordered_set<std::string> checked_locations;
+ for (auto& [resource_id, accessor] : accessor_map_) {
Review Comment:
[P2] Exclude nested vault namespaces from the parent scan
Two valid S3 vaults on the same endpoint/bucket can have prefixes `root` and `root/data`: FE requires a nonempty root, and `create_object_info_with_encrypt` preserves it without adding isolation. A healthy child-vault object `root/data/data/10001/R_0.dat` is also returned by the parent's `list_directory("data")`. After the parent strips `root/`, the checker receives `data/data/10001/R_0.dat`; both file validators parse the second `data` as tablet 0 and return `-1`. The child subsequently checks its live object successfully, but `check_ret` remains `-1`, so every round fails on healthy data regardless of accessor order. Equal-location deduplication cannot handle this overlap. Partition overlapping vault subtrees so each object is validated with its owning relative prefix, and add a healthy nested-prefix case.
##########
cloud/src/recycler/checker.cpp:
##########
@@ -946,16 +940,19 @@ int InstanceChecker::do_inverted_check() {
return -1;
}
- if (tablet_rowsets_cache.tablet_id == tablet_id) {
- if (tablet_rowsets_cache.rowset_ids.contains(rowset_id)) {
- return 0;
- } else {
- LOG(WARNING) << "rowset not exists, key=" << obj_key;
- return -1;
- }
+ if (tablet_rowsets_cache.tablet_id == tablet_id &&
Review Comment:
[P2] Match rowset liveness to the scanned storage location
With two distinct vault locations A/B, commit rowset R of tablet T with `resource_id=A`, then leave an extra `data/T/R_0.dat` copy in B. Both objects pass: the full tablet read stores R without its resource, and this cache accepts R in either accessor. Even a fresh lookup in B accepts A's metadata, so the check returns success and counts no leak. The forward check and recycler select `rs_meta.resource_id()` (`checker.cpp:687`, `recycler.cpp:3912`), so B's copy is unreferenced and will not be removed when A's rowset is recycled. Carry the scanned location into metadata validation and cache identity, accepting alias resources only when they resolve to the same storage. Add a cross-vault orphan-copy case; the new tests use disjoint tablets and miss it.
##########
cloud/src/recycler/s3_accessor.cpp:
##########
@@ -310,8 +310,12 @@ std::optional<S3Conf> S3Conf::from_obj_store_info(const ObjectStoreInfoPB& obj_i
return s3_conf;
}
+// The location is endpoint, bucket and prefix as configured; `uri_` normalizes them for display.
+// '\n' never occurs in an endpoint or a bucket name, so different confs never collide.
S3Accessor::S3Accessor(S3Conf conf)
- : StorageVaultAccessor(AccessorType::S3), conf_(std::move(conf)) {}
+ : StorageVaultAccessor(AccessorType::S3,
+ conf.endpoint + '\n' + conf.bucket + '\n' + conf.prefix),
Review Comment:
[P2] Distinguish S3 buckets selected by the credential context
Endpoint/bucket/prefix equality does not prove namespace equality for supported S3-compatible gateways. [Ceph RGW multi-tenancy](https://docs.ceph.com/en/quincy/radosgw/multitenancy/) permits the same bucket name in different tenants and resolves an unqualified bucket using the authenticated user's tenant. Two path-style S3 vaults can therefore share `endpoint=http://rgw:7480`, `bucket=doris`, and `prefix=warehouse`, while their different AK/SK select disjoint buckets. Vault creation permits both configurations and the accessor supplies their separate credentials to the clients, but these location strings are equal: the checker skips one tenant entirely and can return success despite leaks there. Unlike the earlier normalized-prefix collision, all three strings here are identical. Deduplicate only when the effective namespace is proven equal, conservatively retaining authentication-context distinctions, and add a tenant-aware client fixture.
##########
cloud/src/recycler/hdfs_accessor.cpp:
##########
@@ -325,11 +327,44 @@ class HdfsListIterator final : public ListIterator {
std::vector<DirEntries> level_dirs_;
};
-HdfsAccessor::HdfsAccessor(const HdfsVaultInfo& info)
- : StorageVaultAccessor(AccessorType::HDFS), info_(info), prefix_(info.prefix()) {
- if (!prefix_.empty() && prefix_[0] != '/') {
- prefix_.insert(prefix_.begin(), '/');
+namespace {
+
+// Either empty or starts with '/', e.g. "p1" and "/p1" are the same directory
+std::string accessor_prefix(const HdfsVaultInfo& info) {
+ std::string prefix = info.prefix();
+ if (!prefix.empty() && prefix[0] != '/') {
+ prefix.insert(prefix.begin(), '/');
}
+ return prefix;
+}
+
+// fs_name and prefix name the directory, but a logical nameservice in fs_name resolves through the
+// vault's hdfs_confs: two vaults with fs_name hdfs://ns and prefix /warehouse are different
+// clusters when their dfs.namenode.rpc-address.ns.* differ. Append the effective confs, sorted by
+// key, so equal locations mean equal configurations; differences that do not matter only cost a
+// second listing. '\n' separates the parts, so e.g. hdfs://ns/a + /b and hdfs://ns + /a/b differ.
+std::string accessor_location(const HdfsVaultInfo& info) {
+ // libhdfs applies builder confs so that the first value set for a key wins: keep that one
+ std::map<std::string, std::string> confs;
+ for (const auto& conf : info.build_conf().hdfs_confs()) {
+ confs.emplace(conf.key(), conf.value());
Review Comment:
[P2] Preserve repeated-key semantics on native HDFS builds
Keeping the first value matches Hadoop libhdfs, but the supported MacOS/native branch links libhdfs3 (`cloud/CMakeLists.txt:303-322`). The pinned libhdfs3 [builder](https://github.com/apache/doris-thirdparty/blob/libhdfs3-v2.3.9/src/client/Hdfs.cpp#L563-L570) applies each value immediately, and [Config::set](https://github.com/apache/doris-thirdparty/blob/libhdfs3-v2.3.9/src/common/XmlConfig.h#L164-L170) overwrites the key, so the last value wins. With `enable_distinguish_hdfs_path=false`, create two HA vaults with equal nameservice/prefix/configuration, then ALTER one vault's RPC addresses: `meta_service_resource.cpp:1007-1010` appends the new entries. That native accessor now reaches the new cluster, while this identity keeps the old addresses and deduplicates it with the other vault, hiding leaks. This differs from the earlier unique-key mapping omission. Preserve ordered entries or follow the selected backend's precedence, and cover the native backend.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org