liaoxin01 opened a new pull request, #69705:
URL: https://github.com/apache/doris/pull/69705
### What problem does this PR solve?
Issue Number: None
Related PR: #68348
Problem Summary:
`CachedRemoteFileReader` keeps the downloaded blocks of its file in
`_cache_file_readers` so later reads can skip `get_or_set()`. Those were strong
references, so `FileBlockCell::releasable()` had to discount them with a single
`_owned_by_cached_reader` flag on the block:
```cpp
use_count == 1 || (use_count == 2 && _owned_by_cached_reader)
```
A flag cannot count readers, and the same segment file is often opened by
several readers at once (the one in `SegmentCache`, readers that bypass it, a
concurrent cache miss that loads a second instance). That breaks `releasable()`
in both directions:
1. Blocks stay busy forever. With two readers `use_count` is 3. When one of
them is destroyed it clears the flag although the other still holds the block,
leaving `use_count == 2` with the flag off. Either way a sync `clear` (and LRU
eviction) can only mark the block deleting, and #68348's background recycle
cannot reclaim it either, because it relies on `releasable()` too. The block,
its file and its bytes in `ttl_cache_size` stay until the long-lived reader
goes away, which for a reader in `SegmentCache` means until the tablet is
cleaned up. In a cloud regression run, 899 blocks (929,565,612 bytes of TTL
cache) survived every sync clear for over 12 minutes, failing
`create_table_as_select`, `alter_ttl_1`, `test_ttl_preempt` and
`test_warm_up_cluster_bigsize`.
2. Busy blocks look free. `get_blocks_by_key()` also set the flag for
callers that are not readers (`BlockFileCacheTtlMgr`, cache size statistics,
the file cache HTTP action), and nothing ever cleared it. A block later held
only by a `FileBlocksHolder` of a running query then counts as releasable, so
eviction or clear can delete its file mid-read.
This PR stores `std::weak_ptr` in `_cache_file_readers` and removes the
flag. Readers no longer count as users, so `releasable()` is just `use_count ==
1`, however many readers know about a block. A direct read locks the weak
reference for the duration of the read, which keeps the block from being
deleted under it, and falls back to `get_or_set()` once the cache has dropped
the block. `_insert_file_reader()` now replaces a stale entry at the same
offset instead of keeping it, so direct reads of that range work again after it
is cached anew.
The PR also fixes four cloud file cache cases that failed in the same run
for reasons of their own:
- `test_file_cache_info`: the read after clear was served from the page
cache, so the file cache was never refilled. Disable the page cache for the
session.
- `alter_ttl_4`: after eviction, a full scan rereads the data pages but not
the footers and indexes, which stay in memory with the open segments, so the
reloaded ranges never equal what the load cached. Disable the page cache and
require the evicted files to have more cached data than right after the
eviction.
- `test_ttl_lru_evict`: 10 fixed loads did not fill the TTL queue on a
larger cache, so nothing was evicted. Keep loading until eviction happens,
bounded by the cache capacity, with a TTL long enough to outlast the loads, and
poll for the TTL queue to drain instead of sleeping for a fixed time.
- `test_warm_up_cluster_batch`: BE reports a partition again only when its
queries per day grew by 5 or its last access moved by 60 seconds. One scan per
batch met neither, so the hotspot never refreshed. Scan each table 5 times per
batch.
### Release note
Fix file cache blocks that stay busy after `clear` or eviction when several
readers of the same segment file are open, and blocks that could be deleted
while a query was reading them.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [ ] 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? -->
- Behavior changed:
- [ ] No.
- [x] Yes. Blocks that are only known to `CachedRemoteFileReader` no
longer count as in use, so `clear` and eviction remove them right away; a
reader whose block was removed reads the range through the cache again.
- 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 -->
--
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]
