liaoxin01 commented on PR #68090: URL: https://github.com/apache/doris/pull/68090#issuecomment-5699152372
Thanks for the review. Two of the findings were real defects in this PR and are now fixed; the rest are pre-existing gaps that I would rather not fold into this change. Details below. ## Fixed **Score-runtime read context (P2, tablet_reader.cpp comment).** Correct and this was a genuine miss on my side. `OlapScanner::_prepare_impl()` passed `tablet->ttl_seconds()` positionally into `build_score_runtime_collection_io_context()`, whose parameter is already named `expiration_time`, so inverted index reads for scoring kept persisting the raw duration. Now passes `tablet->file_cache_ttl_expiration_time()`. I also re-audited every remaining `ttl_seconds()` call site in `be/src`; the only ones left are the proto field copies in `pb_convert.cpp`, `BlockFileCacheTtlMgr`'s own use, `CloudTablet::sync_meta()`'s property comparison, and the unrelated cluster-snapshot TTL. **Regression test did not prove the fix (P2, test_ttl_expired_tablet.groovy comment).** Also correct, and the reasoning is exactly right: the suite set `file_cache_background_ttl_gc_interval_ms` to 1000, so under the old code the blocks were admitted as TTL and then demoted within a second or two, and every assertion still held with the production change reverted. The test has been rewritten to run with both TTL background threads at a 10 minute interval, which removes the sweep as a confounder and makes the observed cache type purely the one chosen at admission. It now covers three cases: a load into an already expired tablet, a read of that tablet on a deliberately emptied cache (verified empty first, and using `sum()` rather than `count(*)` so the query actually reads column data instead of hitting metadata), and a tablet still inside its window that must still receive TTL blocks, which guards against the opposite regression of reporting everything as expired. Each case also as serts that blocks were actually cached, so none of them can pass vacuously. ## Not addressed here, and why These three are all real, but they predate this PR and are not made worse by it. This change is deliberately scoped to giving the deadline a single definition; folding cache-restore migration, tablet-id plumbing and a peer-fill protocol change into it would make it considerably harder to review. I am happy to open follow-up issues. **Migrating persisted expiration metadata (P1).** Confirmed: startup restore goes through `add_cell()` rather than `get_or_set()`, so `register_tablet_id()` is never called for restored blocks and `BlockFileCacheTtlMgr` cannot converge them. That is true both before and after this PR, since the manager decides purely from tablet state and never reads the persisted value. The stale persisted values are inert rather than harmful today. **Tablet id on the synchronous warm-up path (P1).** Confirmed, with one correction: `IOContext` has no `tablet_id` field at all, so this is not a missing copy in `CacheContext(io_ctx)`. The real gap is that `CachedRemoteFileReader::_read_from_indirect_cache()` does not set `cache_context.tablet_id` before `get_or_set()`, unlike the sibling path in the same file which does. Pre-existing, and a one-line fix that deserves its own PR and test. **Peer-server pull-through fill (P2).** Confirmed: `handle_peer_file_cache_block_request()` does the first `get_or_set()` with a default-constructed `io::CacheContext`. Pre-existing. Propagating identity and expiration through that RPC needs a capability/rolling-upgrade story, which is well beyond this change. ## Disagreement **Re-evaluating the deadline at final admission (P1).** The behaviour is real: the deadline is sampled once per load, query or warm-up, so work that spans the deadline can still admit TTL blocks after it passes, and a context that captured zero keeps admitting NORMAL across an ALTER extension. But the window is bounded by the duration of that single operation, and one pass of the reconciler converges it. That is a different order of magnitude from the defect this PR fixes, where an expired tablet re-entered the TTL queue on every admission forever with no terminating condition. I do not think it warrants P1, and normalising on every block admission would put a tablet-meta lookup on the per-block cache path. Better handled separately if it proves to matter in practice. -- 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]
