deardeng opened a new pull request, #69620:
URL: https://github.com/apache/doris/pull/69620
Problem Summary:
1. Peer/S3 race issues S3 GETs that are already useless. launch_s3_race()
rechecks the race only after the hedge delay. Once the S3 leg is submitted to
PeerRaceS3ThreadPool, the peer can win while the task is still queued. The task
then allocates a span-sized buffer, sends the S3 GET, reads the whole span and
only afterwards sees winner == 0 and drops the result. This wastes S3 requests,
network bandwidth and pool threads, and makes later S3 tasks queue longer when
the pool is small.
Fix: do_s3_read() takes race->mtx before allocating the buffer or
counting the read. If the peer has already won, it marks the S3 leg as skipped,
notifies waiters and returns. A new bvar peer_race_s3_skipped_after_queue
counts these skips, which separates them from S3 reads that were already in
flight when the peer won. A new sync point s3_task_start is placed before the
check; the existing s3_before_read stays after it, so existing tests that hold
S3 there keep their behavior.
2. Peer cache server uses get_or_set() for plain peer reads.
handle_peer_file_cache_block_request() called BlockFileCache::get_or_set() for
every requested block. For a plain read (no pull-through fill) a miss inserted
an EMPTY cell that was removed again when the holder was released, and the
server could take downloader ownership of a block it never downloads.
Fix: plain peer reads use BlockFileCache::probe(), which never creates
cells or acquires downloader ownership. A null or deleting slot returns
NOT_FOUND so the client rotates the candidate; DOWNLOADING/EMPTY/not-ready
handling and the block read are shared with the fill path. After a successful
read the block is touched with touch_probe_block_if_cached() to keep the LRU
update that get_or_set() used to do. Pull-through fill requests still use
get_or_set(), because the fill download lands in the EMPTY cell it creates and
the handler reads that cell back.
probe() asserts block-aligned offsets, while the request offset comes
from another BE whose cache block size may differ. The handler therefore
rejects an offset that is not a multiple of the cache's block size with
NOT_FOUND instead of reaching the assertion.
BlockFileCache::max_file_block_size() exposes that size. New bvars
file_cache_get_by_peer_probe_{latency,calls, blocks_total} cover the probe
path; the get_or_set bvars now only count fill requests.
A cache hit on the probe path takes the cache mutex three times (probe,
is_block_deleting, touch) instead of once, so this is a semantic cleanup, not a
performance claim.
None
- Test: Unit Test
- New:
CrossCGWinnerRaceTest.s3_task_skips_read_when_peer_won_while_queued,
CrossCGWinnerRaceTest.s3_task_reads_when_started_before_peer_won,
CachedRemoteFileReaderPeerTest.peer_file_cache_handler_probe_miss_returns_not_found,
CachedRemoteFileReaderPeerTest.peer_file_cache_handler_rejects_unaligned_probe_offset;
extended peer_file_cache_handler_allows_cached_block_read
- CachedRemoteFileReaderPeerTest.*, CrossCGWinnerRaceTest.* and
PeerFileCacheLazyRestoreTest: 54 tests passed (ASAN_UT)
- CrossCGWinnerRaceTest.* and peer_file_cache_handler_*: passed with
--gtest_repeat=20
- With the queued-task check disabled,
s3_task_skips_read_when_peer_won_while_queued fails (one S3 read, no skip)
- Behavior changed: Yes. A peer S3 leg that starts after the peer already
won no longer issues an S3 GET. Plain peer cache reads no longer insert EMPTY
cache cells on the serving BE, and unaligned peer block requests return
NOT_FOUND.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] 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:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- 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]
