From a881473fa156669740d93db678feec0484c3f7d0 Mon Sep 17 00:00:00 2001 From: bobhan1 Date: Thu, 29 Oct 2026 10:46:40 +0800 Subject: [PATCH] [fix](be) Validate S3 upload buffer alignment on config updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What problem does this PR solve? Issue Number: None Related PR: #67292, #67579 Problem Summary: File Cache initialization rejects an S3 upload buffer size that is smaller than a cache block or not a multiple of the block size. The buffer size is mutable, but runtime updates did not register the same validation, so a later update could violate the layout assumed by cached reads and hole-fill deduplication. Register the existing alignment constraint as a config validator and cover accepted updates, rejected updates with value rollback, invalid block sizes, and File Cache disabled behavior. Cache probing, read-ahead, and hole-fill algorithms remain unchanged. ### Release note Reject S3 upload buffer configuration updates that violate File Cache block alignment while File Cache is enabled. ### Check List (For Author) - Test: Unit Test — 66 tests from 4 suites pass with ./run-be-ut.sh --run -j100 (ASAN/PCH), including configuration validation and partial-block/async cache writeback. Direct Release/PCH BE build passes with ./build.sh --be -j100. Clang-format 16, build hygiene, and diff checks pass. No cluster or clang-tidy run. - Behavior changed: Yes — invalid runtime S3 upload buffer updates return INVALID_ARGUMENT and retain the previous value; defaults and IO mechanisms are unchanged. - Does this need documentation: No — applies the existing File Cache startup constraint to runtime configuration updates. --- be/src/common/config.cpp | 7 +++++ be/test/common/config_test.cpp | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp index d76d45119ed..98ad1572846 100644 --- a/be/src/common/config.cpp +++ b/be/src/common/config.cpp @@ -1515,6 +1515,13 @@ DEFINE_mInt32(tablet_path_check_batch_size, "1000"); // it must be larger than or equal to 5MB DEFINE_mInt64(s3_write_buffer_size, "5242880"); +// Startup checks this alignment too. Reject runtime updates such as 5.5 MiB with 1 MiB +// cache blocks, which would let upload writers create non-grid cache blocks. +DEFINE_Validator(s3_write_buffer_size, [](int64_t value) { + return !enable_file_cache || + (file_cache_each_block_size > 0 && value >= file_cache_each_block_size && + value % file_cache_each_block_size == 0); +}); // Log interval when doing s3 upload task DEFINE_mInt32(s3_file_writer_log_interval_second, "60"); DEFINE_mInt64(file_cache_max_file_reader_cache_size, "1000000"); diff --git a/be/test/common/config_test.cpp b/be/test/common/config_test.cpp index a1a3de49fd5..21c88578901 100644 --- a/be/test/common/config_test.cpp +++ b/be/test/common/config_test.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -27,6 +28,7 @@ #include "common/status.h" #include "gtest/gtest_pred_impl.h" +#include "util/defer_op.h" namespace doris { using namespace config; @@ -149,4 +151,52 @@ TEST_F(ConfigTest, UpdateConfigs) { EXPECT_EQ(cfg_std_string, "doris_config_test_string"); } +TEST(S3WriteBufferConfigTest, ValidateRuntimeUpdates) { + const bool saved_enable_file_cache = config::enable_file_cache; + const int64_t saved_block_size = config::file_cache_each_block_size; + const int64_t saved_buffer_size = config::s3_write_buffer_size; + auto* saved_config_map = config::full_conf_map; + Defer restore {[&]() { + config::enable_file_cache = saved_enable_file_cache; + config::file_cache_each_block_size = saved_block_size; + config::s3_write_buffer_size = saved_buffer_size; + config::full_conf_map = saved_config_map; + }}; + // Exercise the production registry without changing the configuration dump used by other tests. + config::full_conf_map = nullptr; + constexpr int64_t kMiB = 1024 * 1024; + config::enable_file_cache = true; + config::file_cache_each_block_size = kMiB; + + ASSERT_TRUE(config::set_config("s3_write_buffer_size", "8388608").ok()); + EXPECT_EQ(config::s3_write_buffer_size, 8 * kMiB); + + const int64_t invalid_sizes[] = {-1, 0, kMiB / 2, 5 * kMiB + kMiB / 2}; + for (const auto size : invalid_sizes) { + const auto status = config::set_config("s3_write_buffer_size", std::to_string(size)); + EXPECT_TRUE(status.is()) << status; + EXPECT_EQ(config::s3_write_buffer_size, 8 * kMiB); + } + + // Reject upload buffers smaller than one cache block. + config::file_cache_each_block_size = 8 * kMiB; + EXPECT_TRUE(config::set_config("s3_write_buffer_size", "4194304") + .is()); + EXPECT_EQ(config::s3_write_buffer_size, 8 * kMiB); + ASSERT_TRUE(config::set_config("s3_write_buffer_size", "16777216").ok()); + EXPECT_EQ(config::s3_write_buffer_size, 16 * kMiB); + + // Reject an invalid block size before evaluating the modulo operation. + config::file_cache_each_block_size = 0; + EXPECT_TRUE(config::set_config("s3_write_buffer_size", "8388608") + .is()); + EXPECT_EQ(config::s3_write_buffer_size, 16 * kMiB); + + // Preserve the existing behavior when File Cache is disabled. + config::enable_file_cache = false; + config::file_cache_each_block_size = kMiB; + ASSERT_TRUE(config::set_config("s3_write_buffer_size", "5767168").ok()); + EXPECT_EQ(config::s3_write_buffer_size, 5 * kMiB + kMiB / 2); +} + } // namespace doris