Doris-Breakwater commented on issue #69864: URL: https://github.com/apache/doris/issues/69864#issuecomment-6503374293
The peer-serving recovery gap is supported by the code in both the public **4.1.4 tag** (`ad35a140c7fd`) and **master** (`2a4191c1dc94`). The proposed `NOT_FOUND` handling is appropriate, with one correction to the reported requester-side failure mechanism. This is source-level verification; I have not reproduced it on a running cluster or run regression tests. **Verified behavior** - A failed cache-file read unconditionally sets the protobuf response status to `INTERNAL_ERROR`, then returns the original C++ status. Thus a C++ `NOT_FOUND` becomes an `INTERNAL_ERROR` on the wire. The outer RPC handler preserves that already-set response status; it does not restore `NOT_FOUND`. Neither path invalidates the stale cache entry. See the [4.1.4 handler](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/be/src/cloud/cloud_internal_service.cpp#L763-L771), [master serving helper](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/cloud/cloud_internal_service.cpp#L774-L782), and [outer RPC handler](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/cloud/cloud_internal_service.cpp#L953-L971). Repeated reads can therefore hit the same stale entry until another operation invalidates or evicts it. - The requester routes this response through `update_peer_candidate_on_rpc_failure`, which increments **`consecutive_rpc_failures`** and evicts the candidate for that tablet at **`peer_rpc_failure_eviction_threshold`**. A `NOT_FOUND` instead takes the cache-miss rotation path, with remote fallback available. See the [requester classification](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/cache/cached_remote_file_reader.cpp#L660-L670) and [candidate eviction](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/cloud/cloud_warm_up_manager.cpp#L1325-L1354). - The cited `peer_file_cache_reader.cpp:90` is a different mechanism on master: the address-level **transport** circuit breaker. `transport_succeeded` becomes true before inspecting the response status, so a successfully delivered `INTERNAL_ERROR` does **not** increment that circuit's `consecutive_failures` or trip `cache_peer_read_failure_threshold`. See [transport completion](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/cache/peer_file_cache_reader.cpp#L264-L307). - Local reads already invalidate the file hash on `NOT_FOUND` and fall back to remote storage. [PR #60977](https://github.com/apache/doris/pull/60977) is a public precedent for this recovery policy; it does not fix the peer-serving path. **Suggested implementation and validation** 1. Special-case `read_status.is<ErrorCode::NOT_FOUND>()` in peer serving: invalidate through the cache manager, explicitly set the protobuf status to `NOT_FOUND`, and return the matching C++ status. Preserve error handling for other failures, including I/O errors and short reads. Changing only the returned C++ status is insufficient. 2. Reusing `remove_if_cached_async(hash)` matches local recovery, but it invalidates **all blocks for that file hash**, not only the failed block. Keep that scope explicit in the PR. It marks referenced blocks for deletion; reclamation is deferred. Master plain peer reads hold a `FileBlocks` vector, while fill requests use a holder, so validate both paths and eventual cleanup after references are released/background GC runs. See the [invalidation API](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/cache/block_file_cache.cpp#L1625-L1647) and [deferred cleanup](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/cache/block_file_cache.cpp#L997-L1038). 3. Extend `be/test/io/cache/cached_remote_file_reader_peer_test.cpp`: create a `DOWNLOADED` block with a missing backing file, assert both returned and wire statuses are `NOT_FOUND`, verify eventual removal and successful refill, and verify requester fallback without increasing candidate failure counts. Cover protobuf and attachment responses, a multi-block request that fails after an earlier block succeeds, and a non-`NOT_FOUND` control case. For a deterministic missing-file test, remove the corresponding cached file reader as well as the backing file, or inject `NOT_FOUND` at the storage boundary. Merely unlinking a warmed file can leave an open descriptor readable: [the storage layer checks `FDCache` before opening the path](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/be/src/io/cache/fs_file_cache_storage.cpp#L366-L404). Also ensure BE B actually misses its own cache on each attempt; remote fallback may populate it after the first read. **Evidence still needed** The code establishes the recovery/error-classification gap, but not why the file disappeared. External deletion, a removal race, and a disk fault remain unverified alternatives. Please attach the exact BE build hashes on both sides, a minimal reproduction including how the file and any cached descriptor were removed, and paired BE logs for the same file hash/offset showing the underlying `NOT_FOUND`, returned peer status, and any candidate-eviction message. Include effective peer failure thresholds and whether server-side fill/racing is enabled. This will distinguish tablet-candidate eviction from transport failures and quantify the observed impact. No evidence here establishes data corruption; the demonstrated concern is repeated cache-read failure and unnecessary loss of a usable peer candidate while remote fallback remains available. Breakwater-GitHub-Analysis-Slot: slot_4e1882055a3c -- 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]
