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: [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]
