Doris-Breakwater commented on issue #69289: URL: https://github.com/apache/doris/issues/69289#issuecomment-6267762023
Breakwater-GitHub-Analysis-Slot: slot_149dc6a49809 Source review confirms both reported behaviors at `61179b655805b6ec150cbf7ef50144f8eb6f3ad6`. This is a cloud checker coverage and leak-accounting defect; it does not by itself establish why the recycler left any particular object behind. The issue currently has no labels; cloud/recycler-checker is the relevant ownership area. **Verified findings** - The guard returns success before listing any accessor and before installing the deferred metric updates. The worker treats that return as successful. Therefore a skipped run supplies no evidence that storage is leak-free, and `num_inverted_check_failed` is not refreshed by that run—it can remain at its default or previous value. See [the guard and metrics](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/checker.cpp#L886-L910) and [the caller](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/checker.cpp#L251-L262). - The precise trigger is **more than one entry in `accessor_map_`**, rather than a count of SQL-visible vaults or distinct physical locations. Initialization inserts legacy `instance.obj_info()` resources and configured storage-vault resources, keyed by resource ID. Multiple IDs pointing to one location also trigger the guard. A separate built-in accessor is not unconditionally created. See [initialization](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/checker.cpp#L533-L630). - For a successfully populated tablet cache, an absent rowset returns `-1` on a cache hit and `1` on a miss. Only `1` increments `num_file_leak`. Consecutive orphan segments of one tablet can therefore be undercounted, and a valid segment preceding an orphan can leave the leak count at zero while returning an error. See [cache handling](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/checker.cpp#L949-L1003) and [aggregation](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/checker.cpp#L1052-L1092). **Historical context and patch considerations** [PR #33280](https://github.com/apache/doris/pull/33280/files) did add the guard. Its patch also explicitly skipped HDFS accessors, with a FIXME about excessive listing memory for large tablet counts. That establishes a historical HDFS listing concern, but does **not** establish the reason for the multi-accessor guard. The current [HDFS iterator](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/hdfs_accessor.cpp#L229-L325) traverses recursively and retains directory-entry lists, so HDFS scale deserves validation when enabling broader scans. I found no directly matching historical resolution establishing a different root cause or a ready-made fix. Removing the guard, deduplicating locations, and fixing the cache-hit result is a reasonable direction, with these review points: - Keep resource IDs intact for metadata lookups; deduplicate only the listing pass. [S3 `uri()`](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/src/recycler/s3_accessor.cpp#L424-L430) includes endpoint, bucket and prefix. Exact URI equality handles identical configured locations; it does not prove that differently named endpoints refer to different storage. Do not deduplicate by bucket/prefix alone across different endpoints. - Preserve genuine metadata/listing errors as errors. The tablet cache currently sets its tablet ID before the metadata read completes; a transaction/range-read/parse failure can leave an empty or partial cache. Publish the cache only after a complete successful read, or invalidate it on failure, so changing the hit path to `return 1` cannot turn a failed read into a leak report on the next object. - Coordinate with [#69278](https://github.com/apache/doris/pull/69278): this revision's inverted parser assumes the version-0 key layout. Removing the guard alone does not add support for sharded version-1 paths. Packed files also have a dedicated checker and are deliberately excluded here. **Suggested validation and remaining evidence** Add deterministic tests that execute the actual listing loop: two distinct locations with an orphan in each; two resource IDs with the same URI and a single scan/count; valid-then-orphan and consecutive orphan segments of the same tablet in both listing orders; metadata-read and listing failures. Assert scan/leak counters as well as the return value—`ASSERT_NE(..., 0)` alone cannot distinguish a leak from an error. The existing [segment tests](https://github.com/apache/doris/blob/61179b655805b6ec150cbf7ef50144f8eb6f3ad6/cloud/test/recycler_test.cpp#L5166-L5271) include a normal test that returns through a sync point before listing and a disabled abnormal test, so they do not establish this behavior. Include mixed S3/HDFS and mixed path-format coverage where supported. No further logs are needed to establish these source-level defects. For end-to-end acceptance, please attach the exact tested commits (including any #69278 changes), sanitized resource-ID-to-location mapping and path formats, the injected object keys and listing order, and one full checker round with scan/leak counters. Run against quiescent test data after applicable recycler retention has elapsed; this function checks visible rowset metadata, so absence there alone does not prove that a newly written or pending-recycle object is an actual recycler failure. Confirm older release branches separately before selecting backports; I have verified the cited master revision, not the claim that every 2.1/3.0 branch is unchanged. Validation performed: source and public PR-history inspection; no Doris build, unit-test execution, or cluster reproduction. No repository code was changed. -- 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]
