github-actions[bot] commented on code in PR #67375:
URL: https://github.com/apache/doris/pull/67375#discussion_r3903146978
##########
be/src/storage/rowset/beta_rowset_reader.h:
##########
@@ -53,6 +53,10 @@ class BetaRowsetReader : public RowsetReader {
std::vector<RowwiseIteratorUPtr>* out_iters,
bool use_cache = false) override;
void reset_read_options() override;
+ void set_preferred_file_cache_peer(const std::string& host, int32_t port)
override {
Review Comment:
[P1] Route compaction's pre-reader footer reads through this endpoint
With the default `compaction_batch_size == -1`,
`Merger::vertical_merge_rowsets()` calls `BetaRowset::load_segments()` for
first-time footer sampling before any `get_segment_iterators()` call. That
overload opens segments with a null `IOContext`, so on the cold distributed
worker this feature targets, with global peer reads disabled, those footer
reads still go to object storage even after this setter was called. The
row-ID-conversion fallback similarly calls `get_segment_num_rows(..., _stats)`
without `_read_options.io_ctx`. Please plumb the preference into these
pre-reads and cover cold-cache vertical compaction; the new unit test exercises
only the later lazy-iterator path.
##########
be/src/storage/rowset/beta_rowset_reader.h:
##########
@@ -53,6 +53,10 @@ class BetaRowsetReader : public RowsetReader {
std::vector<RowwiseIteratorUPtr>* out_iters,
bool use_cache = false) override;
void reset_read_options() override;
+ void set_preferred_file_cache_peer(const std::string& host, int32_t port)
override {
+ _read_options.io_ctx.preferred_peer_host = host;
Review Comment:
[P1] Propagate the preferred peer into index compaction
`do_inverted_index_compaction()` reconstructs `IndexFileReader`s directly
from the source rowsets, then calls `init()`, `open()`, and `open_snii_index()`
without an `IOContext`. With global peer reads disabled, cold V1/V2/SNII
source-index reads therefore still use object storage even after the rowset
reader was configured. This phase runs after the row merge and does not reuse
`input_rs_readers`, so fixing the footer pre-reads does not cover it. Please
carry a compaction-level preferred-peer context into every source
index-container read and add a cold-cache index-compaction test.
##########
be/src/storage/rowset/rowset_reader.h:
##########
@@ -58,6 +59,7 @@ class RowsetReader {
std::vector<RowwiseIteratorUPtr>*
out_iters,
bool use_cache = false) = 0;
virtual void reset_read_options() = 0;
Review Comment:
[P1] Route variant-compaction metadata reads through the task peer
With `enable_vertical_compact_variant_subcolumns` enabled by default, both
compaction `build_basic_info()` paths call `get_extended_compaction_schema()`
before the input readers are created. Its variant aggregation opens segments,
column metadata, and external metadata with null/default contexts. Nested-group
streaming-plan construction unwraps configured readers back to rowsets and
repeats those context-free reads, and validation can do so again. On a cold
worker these source reads bypass the preferred coordinator. Please keep the
endpoint in task-level IO state and pass it through variant schema discovery,
streaming planning, and validation, with cold-cache flat/nested variant tests.
##########
be/src/io/cache/cached_remote_file_reader.cpp:
##########
@@ -663,6 +663,19 @@ Status CachedRemoteFileReader::_execute_remote_read(const
std::vector<FileBlockS
return _execute_s3_fallback(empty_start, span_size, buffer,
peer_result, stats, io_ctx);
}
+ if (!io_ctx->preferred_peer_host.empty()) {
Review Comment:
[P1] Bound retries after the preferred peer becomes unreachable
This branch retains the endpoint after every RPC error, so each later
uncached read can wait the 5-second BRPC timeout before falling back to S3. The
ordinary candidate path evicts an endpoint after
`peer_rpc_failure_eviction_threshold` consecutive failures (default 3), but the
preferred branch bypasses that protection. A coordinator outage can therefore
add roughly five seconds per cold operation throughout a compaction. Please add
a task-scoped circuit breaker/cooldown for timeout-class RPC failures while
preserving cache-miss/overload semantics, and add a multi-read outage test.
This is distinct from the existing zero-port thread because the endpoint is
initially valid and fails at runtime.
##########
be/src/storage/rowset/beta_rowset_reader.h:
##########
@@ -53,6 +53,10 @@ class BetaRowsetReader : public RowsetReader {
std::vector<RowwiseIteratorUPtr>* out_iters,
bool use_cache = false) override;
void reset_read_options() override;
+ void set_preferred_file_cache_peer(const std::string& host, int32_t port)
override {
+ _read_options.io_ctx.preferred_peer_host = host;
+ _read_options.io_ctx.preferred_peer_port = port;
Review Comment:
[P1] Preserve the preference when grouped compaction rebuilds readers
The production grouped path in
`CloudCumulativeCompaction::do_merge_input_rowsets()` ignores the configured
`input_rs_readers` vector and calls `input_rowset->create_reader()` for every
segment range. Each fresh reader has an empty preferred endpoint, so with
global peer reads disabled the range's normal segment footer/index/data IO goes
to object storage. This is not the clone or pre-reader-footer case:
`segment_range` skips that sampler and the configured reader is replaced
outright. Please keep this routing state at compaction-task scope and apply it
to every range reader, with a cold-cache grouped-compaction test.
##########
be/src/io/io_common.h:
##########
@@ -251,6 +251,9 @@ struct IOContext {
// Per-call override for cache write completion semantics. An unset value
follows the reader
// option and the global async file-cache write switch.
std::optional<CacheWriteMode> cache_write_mode_override = std::nullopt;
+ // Optional per-request peer; try only this peer before falling back to
remote storage.
+ std::string preferred_peer_host {};
Review Comment:
[P2] Preserve preferred-peer IO in compaction statistics
A successful preferred read is recorded in
`FileCacheStatistics::bytes_read_from_peer` and `peer_io_timer`, but both
merger paths export only local/remote bytes and time, and `Compaction` updates
only the local/remote compaction metrics. With global peer reading disabled,
this feature therefore moves input bytes out of the remote category without
adding them anywhere in the compaction task/tracker statistics, so reported
compaction IO drops despite the same data being read. Please propagate peer
bytes/time through horizontal, vertical, and grouped compaction (or
deliberately classify them into an existing category) and add a statistics
assertion.
--
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]
