Copilot commented on code in PR #67375:
URL: https://github.com/apache/doris/pull/67375#discussion_r3902601191
##########
be/src/storage/rowset/rowset_reader.h:
##########
@@ -21,6 +21,7 @@
#include <gen_cpp/olap_file.pb.h>
Review Comment:
`int32_t` is used in the public header but this file doesn’t include
`<cstdint>` (or `<stdint.h>`). Relying on transitive includes can break builds;
add the appropriate standard header so `int32_t` is guaranteed to be defined
here.
##########
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;
+ virtual void set_preferred_file_cache_peer(const std::string& host,
int32_t port) = 0;
Review Comment:
`int32_t` is used in the public header but this file doesn’t include
`<cstdint>` (or `<stdint.h>`). Relying on transitive includes can break builds;
add the appropriate standard header so `int32_t` is guaranteed to be defined
here.
##########
be/src/io/cache/cached_remote_file_reader.cpp:
##########
@@ -169,7 +169,7 @@ bool
CachedRemoteFileReader::_can_read_cache_file_directly() const {
bool CachedRemoteFileReader::_should_read_from_peer(const IOContext* io_ctx)
const {
return doris::config::is_cloud_mode() && _is_doris_table && _tablet_id > 0
&&
!io_ctx->is_warmup && !io_ctx->bypass_peer_read &&
- doris::config::enable_cache_read_from_peer;
+ (doris::config::enable_cache_read_from_peer ||
!io_ctx->preferred_peer_host.empty());
Review Comment:
`_should_read_from_peer()` enables peer reads when `preferred_peer_host` is
non-empty, but it doesn’t validate `preferred_peer_port`. If a caller sets only
the host (or leaves port at 0), the code will attempt a peer read that is
guaranteed to fail (`execute_peer_read` returns an error for port==0),
generating avoidable warnings and overhead. Require both host non-empty and
port > 0 when using the preferred-peer override.
##########
be/test/io/cache/cached_remote_file_reader_peer_test.cpp:
##########
@@ -573,9 +573,7 @@ TEST_F(CachedRemoteFileReaderPeerTest,
read_at_uses_peer_cache_when_available) {
auto addr = start_peer_test_server(&server, &service);
Defer stop_server {[&]() { stop_peer_test_server(&server); }};
Review Comment:
These tests mutate a global config flag and don’t restore it afterward. This
can leak state into subsequent tests and cause order-dependent failures. Save
the original value and restore it via `Defer`/RAII (or a test fixture
setup/teardown) within each test.
##########
be/test/io/cache/cached_remote_file_reader_peer_test.cpp:
##########
@@ -2119,6 +2120,63 @@ TEST_F(CachedRemoteFileReaderPeerTest,
read_at_falls_back_to_remote_when_peer_re
EXPECT_EQ(cache_stats.bytes_read_from_remote, 10);
}
+TEST_F(CachedRemoteFileReaderPeerTest,
+ read_at_falls_back_to_remote_when_preferred_peer_read_fails) {
+ const std::string content = "abcdefghijklmnop";
+ const fs::path file_path =
+
create_peer_test_file("cached_remote_reader_preferred_peer_fallback.dat",
content);
+ Defer cleanup_file {[&]() {
+ std::error_code ec;
+ fs::remove(file_path, ec);
+ }};
+
+ const fs::path cache_path = caches_dir /
"cached_remote_reader_preferred_peer_fallback_cache";
+ Defer cleanup_cache {[&]() {
+ std::error_code ec;
+ fs::remove_all(cache_path, ec);
+ }};
+
+ clear_cached_remote_reader_factory();
+ create_peer_test_cache(cache_path, kPeerTestBlockSize);
+
+ MockPeerCacheServiceOptions options;
+ options.fail_status = true;
+ MockPeerCacheService service(content, options);
+ brpc::Server server;
+ auto addr = start_peer_test_server(&server, &service);
+ Defer stop_server {[&]() { stop_peer_test_server(&server); }};
+
Review Comment:
These tests mutate a global config flag and don’t restore it afterward. This
can leak state into subsequent tests and cause order-dependent failures. Save
the original value and restore it via `Defer`/RAII (or a test fixture
setup/teardown) within each test.
##########
be/test/io/cache/cached_remote_file_reader_peer_test.cpp:
##########
@@ -2119,6 +2120,63 @@ TEST_F(CachedRemoteFileReaderPeerTest,
read_at_falls_back_to_remote_when_peer_re
EXPECT_EQ(cache_stats.bytes_read_from_remote, 10);
}
+TEST_F(CachedRemoteFileReaderPeerTest,
+ read_at_falls_back_to_remote_when_preferred_peer_read_fails) {
+ const std::string content = "abcdefghijklmnop";
+ const fs::path file_path =
+
create_peer_test_file("cached_remote_reader_preferred_peer_fallback.dat",
content);
+ Defer cleanup_file {[&]() {
+ std::error_code ec;
+ fs::remove(file_path, ec);
+ }};
+
+ const fs::path cache_path = caches_dir /
"cached_remote_reader_preferred_peer_fallback_cache";
+ Defer cleanup_cache {[&]() {
+ std::error_code ec;
+ fs::remove_all(cache_path, ec);
+ }};
+
+ clear_cached_remote_reader_factory();
+ create_peer_test_cache(cache_path, kPeerTestBlockSize);
+
+ MockPeerCacheServiceOptions options;
+ options.fail_status = true;
+ MockPeerCacheService service(content, options);
+ brpc::Server server;
+ auto addr = start_peer_test_server(&server, &service);
+ Defer stop_server {[&]() { stop_peer_test_server(&server); }};
+
+ config::enable_cache_read_from_peer = false;
+
+ FileReaderSPtr local_reader;
+ ASSERT_TRUE(global_local_filesystem()->open_file(file_path.string(),
&local_reader).ok());
+
+ FileReaderOptions opts;
+ opts.cache_type = FileCachePolicy::FILE_BLOCK_CACHE;
+ opts.is_doris_table = true;
+ opts.mtime = 1;
+ opts.tablet_id = kPeerTestTabletId;
+ CachedRemoteFileReader reader(local_reader, opts);
+
+ std::string buffer(10, '#');
+ size_t bytes_read = 0;
+ IOContext io_ctx;
+ io_ctx.preferred_peer_host = "127.0.0.1";
+ io_ctx.preferred_peer_port = addr.port;
+ FileCacheStatistics cache_stats;
+ io_ctx.file_cache_stats = &cache_stats;
+
+ ASSERT_TRUE(reader.read_at(1, Slice(buffer.data(), buffer.size()),
&bytes_read, &io_ctx).ok());
+
+ EXPECT_EQ(buffer, content.substr(1, 10));
+ EXPECT_EQ(bytes_read, 10);
+ EXPECT_EQ(service.rpc_count.load(), 1);
+ EXPECT_EQ(cache_stats.num_peer_io_total, 0);
Review Comment:
This test asserts `service.rpc_count == 1`, which indicates a peer RPC was
attempted, but then expects `cache_stats.num_peer_io_total == 0`. That looks
inconsistent with the meaning of a “total peer IO” counter. Either update the
expectation (likely `num_peer_io_total` should be 1 for an attempted-but-failed
peer read) or adjust the production stats accounting to match the intended
semantics for preferred-peer failures.
--
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]
