Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Reviewed head 5898e994c44a132719bd9751b9c3c00d306f251f across all 16 changed files. No verified new findings and no existing inline blockers. The static review converged in 1 round: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS, and all initial risk items received independently checked dispositions.
Critical checkpoint conclusions:
- Goal and evidence: the async path now reads peer misses, returns the validated peer bytes, and queues shared IOBuf payloads for persistence. Added unit tests cover this behavior; they were inspected, not executed here.
- Scope and reuse: the changes remain focused on async peer support and its profiles/tests. Shared remote dispatch and buffer copy/persistence interfaces are reused.
- Concurrency and locks: query readers, peer bthreads, S3 pool tasks and write workers were traced. Existing race, cache, queue and index locks protect shared state; disk IO and eviction finalizers stay outside the queue lock. No new lock inversion found.
- Lifecycle and memory: request ranges/context captures and reader ownership survive late race completion. Immutable IOBuf wrappers remain owned by tasks/inflight readers; rejection, eviction, stale epochs and shutdown preserve cleanup. Payload charges balance at wrapper destruction; they are logical bytes rather than exact backing-slab/RSS measurements. No new SIOF or ownership cycle found.
- Configuration: no config added; existing mode/peer settings are read per call. Warmup/dryrun precedence and mixed-source peer bypass remain explicit.
- Compatibility: internal callers use the new range interface consistently. No disk-format or new libc/dependency change identified; protobuf responses remain supported for older peers. Final release ELF compatibility was not audited.
- Thrift/protobuf field IDs: not applicable; no definitions changed.
- Parallel paths: sync/async, sequential/race, peer/storage, attachment/protobuf, cache/inflight/downloading, prefetch/warmup and remote-only paths were checked.
- Special conditions and correctness: exact-coverage parsing, original block indexes, sorted regrouping, user-range clipping and EOF write_size support the new assertions. The mixed-hit and whole-range fallback conditions explain their concrete scenarios. No new data-corruption route found.
- Tests: positive and negative cases cover peer success/reorder, miss/error/incomplete fallback, mixed-hit bypass, both race outcomes, multi-segment buffers, short EOF persistence, eviction tracker release and profile aggregation.
- Test results: no regression goldens changed. No builds or unit/regression tests ran; the author's ASAN results were not treated as executed evidence from this review.
- Observability: both new timers propagate through read statistics, merge/delta reporting, scanner detection, profile counters and the FileCache total. Existing source counters and worker error logs remain available.
- Transactions, EditLog and failover: no changes to transaction visibility, partition versions, MoW/delete bitmaps or FE metadata persistence.
- Cache writes and crash behavior: original miss bits, valid EOF sizes, downloader ownership, epoch fences and finalize-before-publication are preserved. Persistence failure remains opportunistic and does not invalidate successful query bytes.
- FE-BE variables: not applicable; none added.
- Performance and BE hygiene: peer tasks share IOBuf payloads and reuse append_iobuf without flattening; task creation adds no per-block payload copy after peer parsing, and user bytes are copied into the result buffer. The read-only
bash build-support/check-build-hygiene.shpassed all checks. No measured performance claim made. - Other concerns and user focus: no additional user focus was supplied. The initial coverage/index, buffer ownership/accounting and late-race lifecycle concerns were closed with code evidence; no issue was withheld or duplicated.
Validation limits: no runtime sanitizer, OOM, exact RSS or load/scheduling measurements were performed. Dedicated async-peer inflight/EOF integration, active-read policy changes, shutdown/invalidation under load and memory-storage retention do not have newly executed proof here. The peer-win tests use an existing timing heuristic, so deterministic scheduling is not claimed. No unresolved PR-specific static candidate or omitted changed-file scope remains.
Review converged after 1 round(s).
…hen misses surround local blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (no candidate, peer failure, or S3 winning the race) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. Two bvars report how often peer is skipped because of the span shape: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans that read remote storage only because of one of the three shape cases above. Spans that try peer but end on remote storage (no candidate, peer miss or failure, S3 winning the race) are not counted. ### Release note None ### Check List (For Author) - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible span without candidates copies only the misses from storage. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*: 276 tests, 273 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The 20 async peer and race cases pass 10 repeated runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF; there is no performance data yet. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. - Does this need documentation: No
b891898 to
0c79ce2
Compare
…hen misses surround local blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (no candidate, peer failure, or S3 winning the race) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. Two bvars report how often peer is skipped because of the span shape: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans that read remote storage only because of one of the three shape cases above. Spans that try peer but end on remote storage (no candidate, peer miss or failure, S3 winning the race) are not counted. ### Release note None ### Check List (For Author) - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible span without candidates copies only the misses from storage. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*: 276 tests, 273 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The 20 async peer and race cases pass 10 repeated runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF; there is no performance data yet. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. - Does this need documentation: No
…hen misses surround local blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (no candidate, peer failure, or S3 winning the race) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. Two bvars report how often peer is skipped because of the span shape: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans that read remote storage only because of one of the three shape cases above. Spans that try peer but end on remote storage (no candidate, peer miss or failure, S3 winning the race) are not counted. Peer byte accounting in the query profile is also corrected. `collect_race_result` and `_execute_sequential_peer_read` added the whole contiguous span size to `bytes_read_from_cross_cg_peer` / `bytes_read_from_same_cg_peer`, although peer returns only the requested ranges. On the sync path, DOWNLOADED blocks between the first and last EMPTY block were therefore counted as peer bytes (an existing overcount), and the sparse async read above would count its locally read middle blocks the same way. Both now add `PeerFetchResult::bytes_read`, the byte count `fetch_blocks()` verified, read before the result is moved out; the unused `span_size` parameter of `collect_race_result` is removed. The race lambda also captures `empty_ranges` and `candidates` by value directly instead of `std::move` on const references, which already copied. ### Release note None ### Check List (For Author) - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible span without candidates copies only the misses from storage. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Peer byte accounting: sparse peer wins assert the CG byte counters equal the 8 bytes of the two missing blocks rather than the 12-byte span, for an async race win with a cross-CG candidate (async_race_sparse_peer_wins_around_cached_block), an async sequential read with a same-CG candidate (async_sequential_sparse_peer_counts_only_miss_bytes), and a sync EMPTY/DOWNLOADED/EMPTY race win (sync_peer_wins_counts_only_empty_block_bytes, which releases the S3 leg and waits for it to drop the reader before returning). All three read 12 before the fix. An all-miss race win still counts 12. - Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*: 286 tests, 283 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The async peer and sync/async race cases (37 tests) pass 10 repeated runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF; there is no performance data yet. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. - Does this need documentation: No
0c79ce2 to
ba75f0e
Compare
…hen misses surround local blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies, the tablet has a peer candidate, and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (peer failure, S3 winning the race, or the candidates disappearing after the check) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. When the tablet has no usable candidate (none fetched yet, or in all-miss cooldown), reading the hits locally would only delay the whole-span storage read that follows, so the sparse read is skipped and the span is reread from storage as before this change. The check uses the new read-only `CloudWarmUpManager::has_peer_candidates()`, which, unlike `get_peer_candidates()`, does not refresh candidate expiry, reset an expired cooldown or touch bvars. Such a span also starts the background candidate fetch from FE (unless in cooldown), through the same helper `_execute_remote_read()` now uses; otherwise a tablet that only reads mixed spans would never fetch candidates. Candidates may change between the check and the read; either way the read falls back to storage, as before. Three bvars report how eligible spans use peer: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_no_candidate_span_count`: eligible spans with hits whose tablet has no usable candidate. This takes precedence over the span shape. - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans with hits and candidates that read remote storage only because of one of the three shape cases above. Eligible spans made purely of misses, and sparse reads, are in neither. Spans that try peer but end on remote storage (peer miss or failure, S3 winning the race) are not counted either. Peer byte accounting in the query profile is also corrected. `collect_race_result` and `_execute_sequential_peer_read` added the whole contiguous span size to `bytes_read_from_cross_cg_peer` / `bytes_read_from_same_cg_peer`, although peer returns only the requested ranges. On the sync path, DOWNLOADED blocks between the first and last EMPTY block were therefore counted as peer bytes (an existing overcount), and the sparse async read above would count its locally read middle blocks the same way. Both now add `PeerFetchResult::bytes_read`, the byte count `fetch_blocks()` verified, read before the result is moved out; the unused `span_size` parameter of `collect_race_result` is removed. The race lambda also captures `empty_ranges` and `candidates` by value directly instead of `std::move` on const references, which already copied. The winner race gets two test sync points, `run_peer_race::peer_succeeded` and `CachedRemoteFileReader::_execute_winner_race::s3_published`, both hit after the leg releases `race->mtx`; the peer side is moved into a small `publish_peer_success()` helper. Race tests that expect a winner now wait for the other leg's signal instead of sleeping. The S3-win copy loop of the sparse read also checks the remote buffer bound. ### Release note None ### Check List (For Author) - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible mixed span without candidates skips the local reads (local bytes 0, remote 12), counts no_candidate and not bypass, and starts the candidate fetch (lazy-fetch counter +1, and the fetch is waited for); the same span in cooldown starts no fetch; a DOWNLOADING middle block without candidates counts no_candidate, not bypass. - CloudWarmUpManagerPeerTest.has_peer_candidates_is_read_only: unknown tablet, empty candidates and active cooldown return false; expired cooldown and present candidates return true; the candidate cache hit/miss bvars, last_access_time_ms and cooldown fields are unchanged. - The ten race tests that expect peer to win wait for peer_succeeded instead of an RPC count plus a 10 ms sleep, and race_peer_stops_when_s3_wins waits for s3_published instead of a 100 ms sleep. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Peer byte accounting: sparse peer wins assert the CG byte counters equal the 8 bytes of the two missing blocks rather than the 12-byte span, for an async race win with a cross-CG candidate (async_race_sparse_peer_wins_around_cached_block), an async sequential read with a same-CG candidate (async_sequential_sparse_peer_counts_only_miss_bytes), and a sync EMPTY/DOWNLOADED/EMPTY race win (sync_peer_wins_counts_only_empty_block_bytes, which releases the S3 leg and waits for it to drop the reader before returning). All three read 12 before the fix. An all-miss race win still counts 12. - Before the no-candidate change the new no-candidate and has_peer_candidates tests fail to build. With only the candidate check in _prepare_async_sparse_peer removed for a local experiment (not committed), async_sparse_span_without_candidate_reads_storage_and_fetches fails on no_candidate (delta 0), bytes_read_from_local (4) and bytes_read_from_remote (8). Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*, CloudWarmUpManagerPeerTest.*: 318 tests, 315 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The async peer and race cases (38 tests) pass 20 repeated runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF; there is no performance data yet. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. - Does this need documentation: No
ba75f0e to
fd80c4e
Compare
…hen misses surround local blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies, the tablet has a peer candidate, and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (peer failure, S3 winning the race, or the candidates disappearing after the check) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. When the tablet has no usable candidate (none fetched yet, or in all-miss cooldown), reading the hits locally would only delay the whole-span storage read that follows, so the sparse read is skipped and the span is reread from storage as before this change. The check uses the new read-only `CloudWarmUpManager::has_peer_candidates()`, which, unlike `get_peer_candidates()`, does not refresh candidate expiry, reset an expired cooldown or touch bvars. Such a span also starts the background candidate fetch from FE (unless in cooldown), through the same helper `_execute_remote_read()` now uses; otherwise a tablet that only reads mixed spans would never fetch candidates. Candidates may change between the check and the read; either way the read falls back to storage, as before. Three bvars report how eligible spans use peer: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_no_candidate_span_count`: eligible spans with hits whose tablet has no usable candidate. This takes precedence over the span shape. - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans with hits and candidates that read remote storage only because of one of the three shape cases above. Eligible spans made purely of misses, and sparse reads, are in neither. Spans that try peer but end on remote storage (peer miss or failure, S3 winning the race) are not counted either. Peer byte accounting in the query profile is also corrected. `collect_race_result` and `_execute_sequential_peer_read` added the whole contiguous span size to `bytes_read_from_cross_cg_peer` / `bytes_read_from_same_cg_peer`, although peer returns only the requested ranges. On the sync path, DOWNLOADED blocks between the first and last EMPTY block were therefore counted as peer bytes (an existing overcount), and the sparse async read above would count its locally read middle blocks the same way. Both now add `PeerFetchResult::bytes_read`, the byte count `fetch_blocks()` verified, read before the result is moved out; the unused `span_size` parameter of `collect_race_result` is removed. The race lambda now init-captures `empty_ranges` and `candidates`, copying each once into a non-const member that the `mutable` lambda moves into `run_peer_race`. Before, it captured the const reference parameters by value, so the members were const and the `std::move` in the lambda copied a second time. The winner race gets two test sync points, `run_peer_race::peer_succeeded` and `CachedRemoteFileReader::_execute_winner_race::s3_published`, both hit after the leg releases `race->mtx`; the peer side is moved into a small `publish_peer_success()` helper. Race tests that expect a winner now wait for the other leg's signal instead of sleeping. The S3-win copy loop of the sparse read also checks the remote buffer bound. PeerRaceS3ThreadPool runs the S3 leg of every peer/S3 winner race, on the sync and async paths alike. Its size, max_peer_race_s3_thread_num, defaults to 32, smaller than the 64 race slots of max_concurrent_peer_races, and its queue is unbounded. With more running races than threads, S3 legs queue behind each other, and raising the race slots alone makes it worse. On a two-BE cluster (both PL1 disks, SSB SF1000 lineorder full scan, async write + peer; peer-async-perf-exp1-results.md): - 64 scanners, where the race slots are not the limit (0 serial peer reads, race peak 63): 64 slots / 32 threads (E0/64/C/1..3) took 169.20 s with an S3 pool queue peak of 16; 256 slots / 256 threads (E1b/64/C/1..3) took 144.28 s (-14.7%) with a queue peak of 0. - 128 scanners: 64 slots / 32 threads (E0/128/C/1..3) took 124.24 s; 256 slots with 32 threads (E1a/128/C/1..3) took 172.51 s (+38.9%) with the queue at 103-264 and about 110 ms wait per S3 task; 256 slots / 256 threads (E1b/128/C/1..3) took 77.42 s (-55.1% vs E1a). The pool is now created with max(max_peer_race_s3_thread_num, max_concurrent_peer_races) threads, so every race slot has an S3 thread, and BE logs the effective size with both config values. Changing max_concurrent_peer_races at runtime does not resize the pool; the config comment says so. ### Release note None ### Check List (For Author) - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible mixed span without candidates skips the local reads (local bytes 0, remote 12), counts no_candidate and not bypass, and starts the candidate fetch (lazy-fetch counter +1, and the fetch is waited for); the same span in cooldown starts no fetch; a DOWNLOADING middle block without candidates counts no_candidate, not bypass. - CloudWarmUpManagerPeerTest.has_peer_candidates_*: four tests (unknown or empty tablet, cooling down, cooldown expired, with candidates). Unknown tablet, empty candidates and active cooldown return false; expired cooldown and present candidates return true; after each call the candidate cache hit/miss bvars, last_access_time_ms and cooldown fields are unchanged. - The ten race tests that expect peer to win wait for peer_succeeded instead of an RPC count plus a 10 ms sleep, and race_peer_stops_when_s3_wins waits for s3_published instead of a 100 ms sleep. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Peer byte accounting: sparse peer wins assert the CG byte counters equal the 8 bytes of the two missing blocks rather than the 12-byte span, for an async race win with a cross-CG candidate (async_race_sparse_peer_wins_around_cached_block), an async sequential read with a same-CG candidate (async_sequential_sparse_peer_counts_only_miss_bytes), and a sync EMPTY/DOWNLOADED/EMPTY race win (sync_peer_wins_counts_only_empty_block_bytes, which releases the S3 leg and waits for it to drop the reader before returning). All three read 12 before the fix. An all-miss race win still counts 12. - Before the no-candidate change the new no-candidate and has_peer_candidates tests fail to build. With only the candidate check in _prepare_async_sparse_peer removed for a local experiment (not committed), async_sparse_span_without_candidate_reads_storage_and_fetches fails on no_candidate (delta 0), bytes_read_from_local (4) and bytes_read_from_remote (8). Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*, CloudWarmUpManagerPeerTest.*: 318 tests, 315 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The async peer and race cases (38 tests) pass 20 repeated runs. - Regression test (docker, cloud): cloud_p0/cache/read_from_peer/test_async_file_cache_write_peer runs two compute groups with async cache writes and the peer/S3 race. It warms CG-A until a query reads nothing from storage, lets CG-B fetch candidates, then checks on CG-B: an empty cache reads its misses from CG-A through peer (peer success, peer race wins, async submissions and eligible spans increase, no no_candidate span); after the async writes finish the same query reads neither peer nor storage; a partially warmed cache returns the same result; with peer read disabled at runtime nothing is read from peer. Every result matches CG-A. Passed on three consecutive runs (one earlier run passed every assertion but its cluster teardown exceeded the framework's 60 s compose timeout). With peer read disabled on CG-B just before the peer step (local experiment, not committed) it fails on the peer success, peer win and eligible span deltas, all 0. - Fixed an empty wait in test_async_file_cache_write: it polled async_cache_write_pending_task_count, which does not exist in be/src, so the wait never waited. It now waits for async_cache_write_buffer_memory_bytes to return to 0. The .out is unchanged and the case passed three runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF. - Performance: tested on a real cluster (see the Performance section of the PR description). With the default race config, fast peer source and PL1 reader: async+peer vs async+S3 −56.6% (64 scanners) / −20.0% (128), S3 GETs about −90%. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. The default effective size of PeerRaceS3ThreadPool grows from 32 to 64 threads (equal to max_concurrent_peer_races), which also affects the peer/S3 race on the sync path. - Does this need documentation: No
fd80c4e to
8895b69
Compare
|
run buildall |
|
run buildall |
TPC-H: Total hot run time: 28461 ms |
TPC-DS: Total hot run time: 150482 ms |
ClickBench: Total hot run time: 24.35 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…hen misses surround local blocks Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies, the tablet has a peer candidate, and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (peer failure, S3 winning the race, or the candidates disappearing after the check) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. When the tablet has no usable candidate (none fetched yet, or in all-miss cooldown), reading the hits locally would only delay the whole-span storage read that follows, so the sparse read is skipped and the span is reread from storage as before this change. The check uses the new read-only `CloudWarmUpManager::has_peer_candidates()`, which, unlike `get_peer_candidates()`, does not refresh candidate expiry, reset an expired cooldown or touch bvars. Such a span also starts the background candidate fetch from FE (unless in cooldown), through the same helper `_execute_remote_read()` now uses; otherwise a tablet that only reads mixed spans would never fetch candidates. Candidates may change between the check and the read; either way the read falls back to storage, as before. Three bvars report how eligible spans use peer: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_no_candidate_span_count`: eligible spans with hits whose tablet has no usable candidate. This takes precedence over the span shape. - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans with hits and candidates that read remote storage only because of one of the three shape cases above. Eligible spans made purely of misses, and sparse reads, are in neither. Spans that try peer but end on remote storage (peer miss or failure, S3 winning the race) are not counted either. Peer byte accounting in the query profile is also corrected. `collect_race_result` and `_execute_sequential_peer_read` added the whole contiguous span size to `bytes_read_from_cross_cg_peer` / `bytes_read_from_same_cg_peer`, although peer returns only the requested ranges. On the sync path, DOWNLOADED blocks between the first and last EMPTY block were therefore counted as peer bytes (an existing overcount), and the sparse async read above would count its locally read middle blocks the same way. Both now add `PeerFetchResult::bytes_read`, the byte count `fetch_blocks()` verified, read before the result is moved out; the unused `span_size` parameter of `collect_race_result` is removed. The race lambda now init-captures `empty_ranges` and `candidates`, copying each once into a non-const member that the `mutable` lambda moves into `run_peer_race`. Before, it captured the const reference parameters by value, so the members were const and the `std::move` in the lambda copied a second time. The winner race gets two test sync points, `run_peer_race::peer_succeeded` and `CachedRemoteFileReader::_execute_winner_race::s3_published`, both hit after the leg releases `race->mtx`; the peer side is moved into a small `publish_peer_success()` helper. Race tests that expect a winner now wait for the other leg's signal instead of sleeping. The S3-win copy loop of the sparse read also checks the remote buffer bound. PeerRaceS3ThreadPool runs the S3 leg of every peer/S3 winner race, on the sync and async paths alike. Its size, max_peer_race_s3_thread_num, defaults to 32, smaller than the 64 race slots of max_concurrent_peer_races, and its queue is unbounded. With more running races than threads, S3 legs queue behind each other, and raising the race slots alone makes it worse. On a two-BE cluster (both PL1 disks, SSB SF1000 lineorder full scan, async write + peer; peer-async-perf-exp1-results.md): - 64 scanners, where the race slots are not the limit (0 serial peer reads, race peak 63): 64 slots / 32 threads (E0/64/C/1..3) took 169.20 s with an S3 pool queue peak of 16; 256 slots / 256 threads (E1b/64/C/1..3) took 144.28 s (-14.7%) with a queue peak of 0. - 128 scanners: 64 slots / 32 threads (E0/128/C/1..3) took 124.24 s; 256 slots with 32 threads (E1a/128/C/1..3) took 172.51 s (+38.9%) with the queue at 103-264 and about 110 ms wait per S3 task; 256 slots / 256 threads (E1b/128/C/1..3) took 77.42 s (-55.1% vs E1a). The pool is now created with max(max_peer_race_s3_thread_num, max_concurrent_peer_races) threads, so every race slot has an S3 thread, and BE logs the effective size with both config values. Changing max_concurrent_peer_races at runtime does not resize the pool; the config comment says so. None - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible mixed span without candidates skips the local reads (local bytes 0, remote 12), counts no_candidate and not bypass, and starts the candidate fetch (lazy-fetch counter +1, and the fetch is waited for); the same span in cooldown starts no fetch; a DOWNLOADING middle block without candidates counts no_candidate, not bypass. - CloudWarmUpManagerPeerTest.has_peer_candidates_*: four tests (unknown or empty tablet, cooling down, cooldown expired, with candidates). Unknown tablet, empty candidates and active cooldown return false; expired cooldown and present candidates return true; after each call the candidate cache hit/miss bvars, last_access_time_ms and cooldown fields are unchanged. - The ten race tests that expect peer to win wait for peer_succeeded instead of an RPC count plus a 10 ms sleep, and race_peer_stops_when_s3_wins waits for s3_published instead of a 100 ms sleep. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Peer byte accounting: sparse peer wins assert the CG byte counters equal the 8 bytes of the two missing blocks rather than the 12-byte span, for an async race win with a cross-CG candidate (async_race_sparse_peer_wins_around_cached_block), an async sequential read with a same-CG candidate (async_sequential_sparse_peer_counts_only_miss_bytes), and a sync EMPTY/DOWNLOADED/EMPTY race win (sync_peer_wins_counts_only_empty_block_bytes, which releases the S3 leg and waits for it to drop the reader before returning). All three read 12 before the fix. An all-miss race win still counts 12. - Before the no-candidate change the new no-candidate and has_peer_candidates tests fail to build. With only the candidate check in _prepare_async_sparse_peer removed for a local experiment (not committed), async_sparse_span_without_candidate_reads_storage_and_fetches fails on no_candidate (delta 0), bytes_read_from_local (4) and bytes_read_from_remote (8). Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*, CloudWarmUpManagerPeerTest.*: 318 tests, 315 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The async peer and race cases (38 tests) pass 20 repeated runs. - Regression test (docker, cloud): cloud_p0/cache/read_from_peer/test_async_file_cache_write_peer runs two compute groups with async cache writes and the peer/S3 race. It warms CG-A until a query reads nothing from storage, lets CG-B fetch candidates, then checks on CG-B: an empty cache reads its misses from CG-A through peer (peer success, peer race wins, async submissions and eligible spans increase, no no_candidate span); after the async writes finish the same query reads neither peer nor storage; a partially warmed cache returns the same result; with peer read disabled at runtime nothing is read from peer. Every result matches CG-A. Passed on three consecutive runs (one earlier run passed every assertion but its cluster teardown exceeded the framework's 60 s compose timeout). With peer read disabled on CG-B just before the peer step (local experiment, not committed) it fails on the peer success, peer win and eligible span deltas, all 0. - Fixed an empty wait in test_async_file_cache_write: it polled async_cache_write_pending_task_count, which does not exist in be/src, so the wait never waited. It now waits for async_cache_write_buffer_memory_bytes to return to 0. The .out is unchanged and the case passed three runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF. - Performance: tested on a real cluster (see the Performance section of the PR description). With the default race config, fast peer source and PL1 reader: async+peer vs async+S3 −56.6% (64 scanners) / −20.0% (128), S3 GETs about −90%. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. The default effective size of PeerRaceS3ThreadPool grows from 32 to 64 threads (equal to max_concurrent_peer_races), which also affects the peer/S3 race on the sync path. - Does this need documentation: No
8895b69 to
aa73a10
Compare
|
run buildall |
TPC-H: Total hot run time: 28783 ms |
TPC-DS: Total hot run time: 151329 ms |
ClickBench: Total hot run time: 24.13 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…te path Related PR: apache#65658 Problem Summary: The async file cache write path added in apache#65658 did not support peer reads. `_resolve_cache_write_mode` forced SYNC_WRITE whenever peer read was enabled, and `_read_async_write_path` set `bypass_peer_read` on a copy of the IOContext. In a cloud deployment with peer read enabled, a cache miss therefore always took the sync path, writing and finalizing cache blocks on the query thread. The existing peer chain is zero-copy: the server reads cache files into an IOBuf, the client cuts the attachment, and the sync path writes it with `append_iobuf` (writev). Routing peer data through the async path must not add a flattening copy. 1. Peer chain takes block ranges `PeerFileCacheReader::fetch_blocks` and the peer dispatch helpers (`execute_peer_read`, `run_peer_race`, `_execute_sequential_peer_read`, `_execute_remote_read`, `_execute_winner_race`) took `std::vector<FileBlockSPtr>` but only read `block->range()`. The async path has no `FileBlock` for its misses, so the chain now takes `std::vector<FileBlock::Range>`. The sync caller converts its empty blocks before dispatch. A peer bthread that loses the race no longer holds `FileBlockSPtr`s. 2. Async write buffers can hold an IOBuf `AsyncCacheWriteManager::wrap_iobuf(capacity, payload)` wraps one cache block of peer payload without copying. It reports `capacity` as `size()`, so pending-byte accounting, the fixed task buffer size check and EOF short-block detection are unchanged. brpc allocates IOBuf blocks with malloc, outside any Doris MemTracker, so the wrapper charges the payload length to the async-write tracker on construction and releases it on destruction. `copy_to()` serves inflight readers and `append_to()` serves worker persistence through `FileBlock::append_iobuf`. Allocated buffers behave as before. 3. Async path reads from peers - Mode resolution no longer depends on peer read, and the async path no longer forces `bypass_peer_read`. - A peer fails the whole RPC when it misses any requested block, so peer is only tried when every block in the remote span is a real cache miss. A span that rereads local hits (miss-hit-miss, or the whole-range fallback after a side race) goes straight to remote storage. - On a peer win, chunks are copied to the user result from their IOBufs, regrouped by reference into one IOBuf per span block and wrapped as task buffers. The inflight index and the write worker use the received IOBuf blocks directly; the copy into the user buffer is the only copy. Storage results keep the existing tracked-buffer copy. - Peer bytes copied to the user are accounted as `peer_bytes`. 4. Profile timers Remote IO is already reported as `PeerIOUseTimer` / `RemoteIOUseTimer`. Add `AsyncCopyToUserTimer` (copying the remote/peer span into the user buffer) and `AsyncSubmitWriteTimer` (`_submit_async_write_tasks`) to the file cache profile, so a peer-miss read on the async path can be broken down and compared with the sync path's `WriteCacheIOUseTimer`. Both are included in the file cache group total. No metric or bvar is added. The async file cache write path (`enable_async_file_cache_write`) now reads missing blocks from peer BEs when `enable_cache_read_from_peer` is enabled, instead of falling back to synchronous cache writes. Add AsyncCopyToUserTimer and AsyncSubmitWriteTimer to the file cache profile. - Test: Unit Test - AsyncCacheWriteManagerTest: a multi-segment IOBuf payload is readable across segment boundaries, persisted intact, and its tracker charge returns to baseline; a short IOBuf payload persists the EOF block; an IOBuf task evicted by drop-oldest is finalized on the submitting thread and releases its tracker charge. - CachedRemoteFileReaderAsyncPeerTest (peer injected by debug point, sequential path): peer data is returned and persisted from the IOBuf payload (second read is local and peer-sourced); reordered attachments; a miss-hit-miss span bypasses peer; peer NOT_FOUND, peer error and an incomplete peer response fall back to remote storage and still submit async writes; profile timers are recorded on a peer miss and stay zero on a local read. - CrossCGAsyncWinnerRaceTest (default peer/S3 winner race): peer wins and its payload is persisted; S3 wins when peer fails; S3 wins while a slow peer is still in flight. - FileCacheProfileReporterTest covers the new profile fields. - cache_write_mode_is_resolved_for_each_read_context now expects ASYNC_WRITE with peer read enabled; async_write_keeps_direct_remote_after_peer_policy_change is removed because the behavior it covered no longer exists. Existing fetch_blocks tests pass ranges. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*, BlockFileCacheTest.cache_write_mode_is_resolved_for_each_read_context (113 tests, ASAN) all pass - Behavior changed: Yes. With both async cache write and peer read enabled, cache misses use the async write path and may be served by peers. - Does this need documentation: No
…hen misses surround local blocks Issue Number: N/A Related PR: apache#69071 Problem Summary: A peer fails the whole RPC when it misses any requested block. So on the async cache write read path, a remote span with cached or inflight blocks between its misses (miss-hit-miss) bypassed peer and was reread entirely from remote storage, while the sync path requests only its empty blocks from peer and reads the blocks in between locally. Align the async path with the sync path. When peer read applies, the tablet has a peer candidate, and the non-miss blocks inside the span are all CACHE or INFLIGHT: - Those blocks are read locally first with `_materialize_async_block` (CACHE blocks are touched in the LRU). Their bytes count as local only when all of them succeed. - Peer receives only the miss ranges. On a peer win the misses are copied into the result and their IOBufs are placed at the misses' span positions, so `_submit_async_write_tasks` is unchanged. - A storage fallback (peer failure, S3 winning the race, or the candidates disappearing after the check) still reads the whole span, from which only the misses are copied. The span keeps the existing whole-span storage read in three cases: it contains a DOWNLOADING block (the async path never waits on another downloader), it is the whole-range fallback after a side race (marked on the plan), or a local read inside the span fails; a NOT_FOUND there still triggers self-heal. When peer read does not apply, the path is unchanged. When the tablet has no usable candidate (none fetched yet, or in all-miss cooldown), reading the hits locally would only delay the whole-span storage read that follows, so the sparse read is skipped and the span is reread from storage as before this change. The check uses the new read-only `CloudWarmUpManager::has_peer_candidates()`, which, unlike `get_peer_candidates()`, does not refresh candidate expiry, reset an expired cooldown or touch bvars. Such a span also starts the background candidate fetch from FE (unless in cooldown), through the same helper `_execute_remote_read()` now uses; otherwise a tablet that only reads mixed spans would never fetch candidates. Candidates may change between the check and the read; either way the read falls back to storage, as before. Three bvars report how eligible spans use peer: - `cached_remote_file_reader_async_peer_eligible_span_count`: async-path remote spans that may use peer read (peer read enabled and not bypassed by the caller). - `cached_remote_file_reader_async_peer_no_candidate_span_count`: eligible spans with hits whose tablet has no usable candidate. This takes precedence over the span shape. - `cached_remote_file_reader_async_peer_bypass_span_count`: eligible spans with hits and candidates that read remote storage only because of one of the three shape cases above. Eligible spans made purely of misses, and sparse reads, are in neither. Spans that try peer but end on remote storage (peer miss or failure, S3 winning the race) are not counted either. Peer byte accounting in the query profile is also corrected. `collect_race_result` and `_execute_sequential_peer_read` added the whole contiguous span size to `bytes_read_from_cross_cg_peer` / `bytes_read_from_same_cg_peer`, although peer returns only the requested ranges. On the sync path, DOWNLOADED blocks between the first and last EMPTY block were therefore counted as peer bytes (an existing overcount), and the sparse async read above would count its locally read middle blocks the same way. Both now add `PeerFetchResult::bytes_read`, the byte count `fetch_blocks()` verified, read before the result is moved out; the unused `span_size` parameter of `collect_race_result` is removed. The race lambda now init-captures `empty_ranges` and `candidates`, copying each once into a non-const member that the `mutable` lambda moves into `run_peer_race`. Before, it captured the const reference parameters by value, so the members were const and the `std::move` in the lambda copied a second time. The winner race gets two test sync points, `run_peer_race::peer_succeeded` and `CachedRemoteFileReader::_execute_winner_race::s3_published`, both hit after the leg releases `race->mtx`; the peer side is moved into a small `publish_peer_success()` helper. Race tests that expect a winner now wait for the other leg's signal instead of sleeping. The S3-win copy loop of the sparse read also checks the remote buffer bound. PeerRaceS3ThreadPool runs the S3 leg of every peer/S3 winner race, on the sync and async paths alike. Its size, max_peer_race_s3_thread_num, defaults to 32, smaller than the 64 race slots of max_concurrent_peer_races, and its queue is unbounded. With more running races than threads, S3 legs queue behind each other, and raising the race slots alone makes it worse. On a two-BE cluster (both PL1 disks, SSB SF1000 lineorder full scan, async write + peer; peer-async-perf-exp1-results.md): - 64 scanners, where the race slots are not the limit (0 serial peer reads, race peak 63): 64 slots / 32 threads (E0/64/C/1..3) took 169.20 s with an S3 pool queue peak of 16; 256 slots / 256 threads (E1b/64/C/1..3) took 144.28 s (-14.7%) with a queue peak of 0. - 128 scanners: 64 slots / 32 threads (E0/128/C/1..3) took 124.24 s; 256 slots with 32 threads (E1a/128/C/1..3) took 172.51 s (+38.9%) with the queue at 103-264 and about 110 ms wait per S3 task; 256 slots / 256 threads (E1b/128/C/1..3) took 77.42 s (-55.1% vs E1a). The pool is now created with max(max_peer_race_s3_thread_num, max_concurrent_peer_races) threads, so every race slot has an S3 thread, and BE logs the effective size with both config values. Changing max_concurrent_peer_races at runtime does not resize the pool; the config comment says so. None - Test: Unit Test - New CachedRemoteFileReaderAsyncPeerTest cases: miss-cached-miss with peer winning (peer gets two ranges, middle read locally, two writes submitted, second read local); the same span with peer failing (storage supplies only the misses, no double counting); an inflight middle block; a NOT_FOUND middle block rereads the span and self-heals; a downloading middle block and the whole-range fallback bypass peer. - New CrossCGAsyncWinnerRaceTest cases: sparse peer wins the race; an eligible mixed span without candidates skips the local reads (local bytes 0, remote 12), counts no_candidate and not bypass, and starts the candidate fetch (lazy-fetch counter +1, and the fetch is waited for); the same span in cooldown starts no fetch; a DOWNLOADING middle block without candidates counts no_candidate, not bypass. - CloudWarmUpManagerPeerTest.has_peer_candidates_*: four tests (unknown or empty tablet, cooling down, cooldown expired, with candidates). Unknown tablet, empty candidates and active cooldown return false; expired cooldown and present candidates return true; after each call the candidate cache hit/miss bvars, last_access_time_ms and cooldown fields are unchanged. - The ten race tests that expect peer to win wait for peer_succeeded instead of an RPC count plus a 10 ms sleep, and race_peer_stops_when_s3_wins waits for s3_published instead of a 100 ms sleep. - Split and reordered peer responses: the mock peer service returns every requested block as uneven pieces, interleaved and out of order, for a sparse span and for an all-miss span, in attachment and protobuf-bytes modes. Each case checks the result bytes, the peer/local/remote byte counts, and the cache blocks read back directly after the async writes. With the chunk sort in take_peer_span removed for a local experiment (not committed), all four fail only on the cache read-back ("LIJK", "GHEF", "KLIJ" instead of the peer bytes) while the result bytes stay correct. - Peer byte accounting: sparse peer wins assert the CG byte counters equal the 8 bytes of the two missing blocks rather than the 12-byte span, for an async race win with a cross-CG candidate (async_race_sparse_peer_wins_around_cached_block), an async sequential read with a same-CG candidate (async_sequential_sparse_peer_counts_only_miss_bytes), and a sync EMPTY/DOWNLOADED/EMPTY race win (sync_peer_wins_counts_only_empty_block_bytes, which releases the S3 leg and waits for it to drop the reader before returning). All three read 12 before the fix. An all-miss race win still counts 12. - Before the no-candidate change the new no-candidate and has_peer_candidates tests fail to build. With only the candidate check in _prepare_async_sparse_peer removed for a local experiment (not committed), async_sparse_span_without_candidate_reads_storage_and_fetches fails on no_candidate (delta 0), bytes_read_from_local (4) and bytes_read_from_remote (8). Before the change the new sparse cases fail (no peer RPC, whole span from storage, no self-heal for the middle block) and the counter cases fail to link. - CachedRemoteFileReaderPeerTest.*, CachedRemoteFileReaderAsyncPeerTest.*, CrossCGWinnerRaceTest.*, CrossCGAsyncWinnerRaceTest.*, AsyncCachedRemoteFileReaderTest.*, BlockFileCacheTest.*, AsyncCacheWriteManagerTest.*, InflightWriteBufferIndexTest.*, FileCacheProfileReporterTest.*, CloudWarmUpManagerPeerTest.*: 318 tests, 315 pass and 3 pre-existing DISABLE_ tests are skipped (ASAN). The async peer and race cases (38 tests) pass 20 repeated runs. - Regression test (docker, cloud): cloud_p0/cache/read_from_peer/test_async_file_cache_write_peer runs two compute groups with async cache writes and the peer/S3 race. It warms CG-A until a query reads nothing from storage, lets CG-B fetch candidates, then checks on CG-B: an empty cache reads its misses from CG-A through peer (peer success, peer race wins, async submissions and eligible spans increase, no no_candidate span); after the async writes finish the same query reads neither peer nor storage; a partially warmed cache returns the same result; with peer read disabled at runtime nothing is read from peer. Every result matches CG-A. Passed on three consecutive runs (one earlier run passed every assertion but its cluster teardown exceeded the framework's 60 s compose timeout). With peer read disabled on CG-B just before the peer step (local experiment, not committed) it fails on the peer success, peer win and eligible span deltas, all 0. - Fixed an empty wait in test_async_file_cache_write: it polled async_cache_write_pending_task_count, which does not exist in be/src, so the wait never waited. It now waits for async_cache_write_buffer_memory_bytes to return to 0. The .out is unchanged and the case passed three runs. - Unit tests were run locally with GLIBC_COMPATIBILITY=OFF. - Performance: tested on a real cluster (see the Performance section of the PR description). With the default race config, fast peer source and PL1 reader: async+peer vs async+S3 −56.6% (64 scanners) / −20.0% (128), S3 GETs about −90%. - Behavior changed: Yes. With async cache write and peer read enabled, a span whose misses surround cached or inflight blocks reads those blocks locally and requests only the misses from peer. The default effective size of PeerRaceS3ThreadPool grows from 32 to 64 threads (equal to max_concurrent_peer_races), which also affects the peer/S3 race on the sync path. - Does this need documentation: No
aa73a10 to
2490e65
Compare
|
run buildall |
### Background Async cache writes publish pending buffers to an inflight index. Reads that skip cache population can reuse those buffers when the index covers the requested range. ### What problem does this PR solve? Issue Number: None Related PR: apache#69071 (async peer cache reads) PR Type: Bugfix Problem Summary: A no-write read can crash the BE when it reuses a peer-backed inflight buffer. Use the buffer's copy interface for both heap and IOBuf payloads. ### Details - **Frequency:** Requires a read that skips cache writes and hits an inflight range containing peer data before writeback finishes. - **Root cause:** The inflight fast path calls data(), which has no valid pointer for a peer IOBuf. - **Why existing tests missed it:** The no-write coverage used heap buffers. The peer IOBuf readback coverage waited for writeback. - **Fix:** Use copy_to() for the bounded inflight copy. Gate the write worker in the new unit test so the no-write read must hit the peer IOBuf. Remove the unused fixture accessor and state left by the deleted bypass-policy test, which blocked the Clang build. - **Other paths and branches:** REMOTE_ONLY_ON_MISS uses the same copy. The regular async read path already uses copy_to(). ### Testing On devbtt, 80 ASAN tests passed across CachedRemoteFileReaderAsyncPeerTest, AsyncCachedRemoteFileReaderTest, AsyncCacheWriteManagerTest, and InflightWriteBufferIndexTest. The new no_write_reads_pending_peer_iobuf test passed. The run used GLIBC_COMPATIBILITY=OFF because the local LDB toolchain and Doris compatibility layer produced duplicate symbols at link time with the default setting. Clang-format 16, clang-tidy, and the BE build hygiene checks passed. ### Release note No-write file cache reads can reuse pending peer data without crashing the BE. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Partially verified: 80 ASAN cache unit tests passed with GLIBC_COMPATIBILITY=OFF on devbtt. ### AI Disclosure - Tool: Codex - Generated: Code, unit test, and commit message - Human review: Pending
|
run buildall |
What problem does this PR solve?
Issue Number: None
Related PR: #65658
Problem Summary:
The async file cache write path (#65658) never read from peers: with peer read enabled,
_resolve_cache_write_modefell back to SYNC_WRITE, and the async path forcedbypass_peer_read. So a cluster could use async write (for slow cache disks) or peer cache, but not both.This PR lets the async path read its misses from peers. Peer data stays zero-copy: the RPC attachment IOBuf goes into the inflight index and is persisted with
FileBlock::append_iobuf; the only copy is into the user buffer.PeerFileCacheReader::fetch_blocksand the peer dispatch helpers takestd::vector<FileBlock::Range>instead ofFileBlockSPtr, because the async path has noFileBlockfor its misses.AsyncCacheWriteManager::wrap_iobuf()wraps one peer block without copying and charges it to the async-write MemTracker.PeerRaceS3ThreadPoolruns the S3 leg of every peer/S3 race (sync and async). It had 32 threads for 64 race slots and an unbounded queue; it is now sizedmax(max_peer_race_s3_thread_num, max_concurrent_peer_races), 64 by default.BytesReadFrom{Cross,Same}CGPeercounted the whole span instead of the bytes peer returned, which also overcounted on the sync path.AsyncCopyToUserTimer,AsyncSubmitWriteTimer.cached_remote_file_reader_async_peer_{eligible,no_candidate,bypass}_span_count.Performance
Cold full scan of SSB SF1000
lineorder(6.0 B rows, 17 columns, 135.8 GB). One source BE with a warm cache and one reader BE, in different compute groups. Default race config. Medians of 3 runs.¹ Tuned race config (256 slots / 256 S3 threads), not rerun at the default.
Method and race-config evidence
drop_cacheson the source (not done in the PL1 → PL1 round);Release note
The async file cache write path (
enable_async_file_cache_write) now reads missing blocks from peer BEs whenenable_cache_read_from_peeris enabled.PeerRaceS3ThreadPoolnow uses at leastmax_concurrent_peer_racesthreads (64 by default, was 32). New profile counters: AsyncCopyToUserTimer, AsyncSubmitWriteTimer. New bvars: cached_remote_file_reader_async_peer_eligible_span_count, cached_remote_file_reader_async_peer_no_candidate_span_count, cached_remote_file_reader_async_peer_bypass_span_count.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)