github-actions[bot] commented on code in PR #69620:
URL: https://github.com/apache/doris/pull/69620#discussion_r4386501524
##########
be/src/cloud/cloud_internal_service.cpp:
##########
@@ -765,103 +861,40 @@ Status handle_peer_file_cache_block_request(const
PFetchPeerDataRequest* request
int sleep_ms = dp->param<int>("sleep_ms", 300);
bthread_usleep(sleep_ms * 1000);
});
- auto get_or_set_start = std::chrono::steady_clock::now();
- auto holder = cache->get_or_set(hash, offset, size, ctx);
- g_file_cache_get_by_peer_get_or_set_latency <<
elapsed_us(get_or_set_start);
- ++get_or_set_calls;
- get_or_set_blocks += holder.file_blocks.size();
-
- for (auto& fb : holder.file_blocks) {
- auto fb_state = fb->state();
- if (fb_state == io::FileBlock::State::DOWNLOADING) {
- if (do_fill) {
- // Only peer fill requests should wait longer here. Plain
peer-cache reads keep
- // the short wait semantics so they can fail fast and let
the client race S3.
- [[maybe_unused]] const bool completed =
wait_for_file_block_state(
- fb, config::peer_server_cache_fill_timeout_ms);
- fb_state = fb->state();
- } else {
- // Wait for in-progress download to complete using the
normal short timeout.
- fb_state = fb->wait();
- }
- }
- if (fb_state == io::FileBlock::State::EMPTY) {
- if (!do_fill) {
- const std::string msg =
- fmt::format("cache block not downloaded, {}",
-
format_peer_cache_block_context(request, cb_req, fb, hash,
-
file_size, do_fill));
- g_file_cache_get_by_peer_failed_num << 1;
- g_file_cache_get_by_peer_not_downloaded_block_num << 1;
- LOG(WARNING) << msg;
- // Use NOT_FOUND so the client can distinguish "block not
cached"
- // from an actual RPC/server error. On NOT_FOUND the
client rotates
- // the candidate to the end of its list (trying another CG
next time)
- // rather than incrementing the RPC-failure eviction
counter.
- response->mutable_status()->add_error_msgs(msg);
-
response->mutable_status()->set_status_code(TStatusCode::NOT_FOUND);
- return Status::NotFound<false>(msg);
- }
- // Server-side fill: request.path already carries the full
remote path.
- auto fill_st = trigger_peer_server_fill(
- fb, get_rowset_meta_tablet_id_from_request(request),
cache_key_path,
- get_rowset_meta_resource_id_from_request(request),
path,
- request->has_file_size() ? request->file_size() : -1,
- static_cast<int64_t>(fb->range().left),
- static_cast<int64_t>(fb->range().size()),
- config::peer_server_cache_fill_timeout_ms);
- if (!fill_st.ok()) {
- g_file_cache_get_by_peer_failed_num << 1;
- g_file_cache_get_by_peer_not_downloaded_block_num << 1;
- if (fill_st.is<ErrorCode::TOO_MANY_TASKS>()) {
- // Server slot exhausted: healthy but overloaded.
Client must not rotate
- // or evict — just fall back to S3 and retry same
candidate next time.
-
response->mutable_status()->add_error_msgs(std::string(fill_st.msg()));
-
response->mutable_status()->set_status_code(TStatusCode::TOO_MANY_TASKS);
- } else if (fill_st.is<ErrorCode::NOT_FOUND>()) {
- // Soft miss (fill incomplete, timeout, unexpected
state) — client rotates,
- // not evicts.
-
response->mutable_status()->add_error_msgs(std::string(fill_st.msg()));
-
response->mutable_status()->set_status_code(TStatusCode::NOT_FOUND);
- } else {
- LOG(WARNING) << "cache block fill failed, status=" <<
fill_st << ", "
- <<
format_peer_cache_block_context(request, cb_req, fb, hash,
-
file_size, do_fill);
- set_error_response(response, "cache block not ready");
- }
- return fill_st;
- }
- fb_state = io::FileBlock::State::DOWNLOADED;
- }
- if (fb_state != io::FileBlock::State::DOWNLOADED) {
- // A concurrent download was in progress (DOWNLOADING at
request time) but its
- // wait() returned a non-DOWNLOADED state (e.g., timed-out
while still
- // DOWNLOADING, or some other non-EMPTY intermediate state).
The server is
- // healthy; the block just isn't available yet. Return
NOT_FOUND so the client
- // rotates the candidate instead of evicting it.
- const std::string msg =
- fmt::format("cache block not ready after wait, {}",
- format_peer_cache_block_context(request,
cb_req, fb, hash,
- file_size,
do_fill));
- g_file_cache_get_by_peer_failed_num << 1;
- g_file_cache_get_by_peer_not_downloaded_block_num << 1;
- LOG(WARNING) << msg;
- response->mutable_status()->add_error_msgs(msg);
-
response->mutable_status()->set_status_code(TStatusCode::NOT_FOUND);
- return Status::NotFound<false>(msg);
+ if (do_fill) {
+ // Pull-through fill needs the EMPTY cell from get_or_set: the
fill download lands in
+ // that cell, and this handler reads it back once the download
finishes.
+ auto get_or_set_start = std::chrono::steady_clock::now();
+ auto holder = cache->get_or_set(hash, offset, size, ctx);
+ g_file_cache_get_by_peer_get_or_set_latency <<
elapsed_us(get_or_set_start);
+ ++get_or_set_calls;
+ get_or_set_blocks += holder.file_blocks.size();
+ for (auto& fb : holder.file_blocks) {
+ RETURN_IF_ERROR(serve_block(cb_req, fb));
}
+ continue;
+ }
- g_file_cache_get_by_peer_blocks_num << 1;
- doris::CacheBlockPB* out = response->add_datas();
- // In attachment mode, metadata order must match attachment append
order because the
- // client consumes attachment payload sequentially using
resp.datas() order.
- Status read_status = read_file_block(
- fb, file_size, out, use_attachment ?
&cntl->response_attachment() : nullptr);
- if (!read_status.ok()) {
- set_error_response(response, "read cache file error");
- return read_status;
- }
- ++response_blocks;
+ // A plain peer read only serves blocks that are already downloaded.
This lookup inserts no
+ // EMPTY cells, takes no downloader ownership, and accepts any cached
layout, including
+ // short cells left by a pull-through fill from a client with a
smaller cache block size.
+ // It also refreshes LRU state for the returned blocks.
+ auto lookup_start = std::chrono::steady_clock::now();
+ io::FileBlocks blocks;
+ bool fully_covered = false;
+ RETURN_IF_ERROR(cache->get_downloaded_blocks_if_fully_covered(hash,
offset, size, ctx,
Review Comment:
[P2] Preserve asynchronous LRU touches for plain peer hits
With `enable_file_cache_async_touch_on_get_or_set=true`, this lookup calls
`use_cell()`, which skips `queue.move_to_end()` in that mode, but neither the
helper nor this handler enqueues `add_need_update_lru_block()`. The previous
`get_or_set()` path did enqueue downloaded hits. For a full NORMAL cache with A
older than B, repeated peer-only reads of A now leave A at the eviction head
even after the LRU worker runs; adding C evicts A before untouched B because
`find_evict_candidates()` ignores `atime`. This regresses hit retention and
causes avoidable S3 reads in deployments using async touch. Preserve async
updates for the returned blocks, for example through
`touch_probe_block_if_cached()`, and cover LRU ordering with the flag enabled.
--
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]
