This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new b180f1443da [improvement](filecache) Enable file cache by default
(#68797)
b180f1443da is described below
commit b180f1443da477ed8c4fde0b8f271cd2a7381a7d
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Mon Oct 12 17:46:44 2026 +0800
[improvement](filecache) Enable file cache by default (#68797)
### What problem does this PR solve?
Issue Number: None
Related PR: #66684
Original author: @hubgeter (the change is ported from #66684 on
branch-4.1; the commit carries Co-authored-by)
Problem Summary:
**In short.** branch-4.1 (from 4.1.5) and branch-4.2 enable the block
file cache by default. master still has it off. This difference matters
most on upgrade, through the BE config: BE configs are not persisted. A
storage-compute-coupled cluster that relies on the default therefore
loses its file cache when it moves from 4.1.5 / 4.2 to a release built
from master. This PR ports #66684 and makes both the BE config
`enable_file_cache` and the session variable `enable_file_cache` default
to true.
**Background**
- Two switches gate the block file cache:
- The BE config `enable_file_cache` decides whether a BE creates the
cache at all. In cloud mode, BE forces it on after reading its config
file and refuses to start without it.
- The session variable `enable_file_cache` decides whether external file
scans read through the cache. `FileFactory::get_reader_options` requires
the BE config, the session variable and file cache admission to all be
true. The session variable has no effect on internal tables.
- With the session variable on, FE also assigns external scan ranges by
consistent hashing instead of round robin (`ExternalScanNode`). A file
then keeps landing on the same backends, so its cached blocks get
reused.
- Session variables and BE configs behave differently across upgrades:
- FE writes every session variable value into its image
(`VariableMgr.write` → `SessionVariable.toJson`). An upgraded cluster
keeps the values it had whatever the new default is; only a new cluster
gets a new default.
- BE configs are not persisted. A BE uses the default compiled into its
binary unless be.conf sets the key.
**The problem, and what it cost**
| Default | master | branch-4.2 | 4.1.4 | 4.1.5 |
|---|---|---|---|---|
| BE config `enable_file_cache` | false | true | false | true |
| session variable `enable_file_cache` | false | true | false | true |
- Take a storage-compute-coupled cluster whose be.conf does not set
`enable_file_cache`. When it is upgraded from 4.1.5 / 4.2 to a release
built from master, it silently loses its file cache. External table
scans and reads of rowsets cooled down to remote storage go back to
remote storage on every query. The `${DORIS_HOME}/file_cache` directory
is left on disk.
- New clusters built from master behave differently from new 4.1.5 / 4.2
clusters. In coupled mode they have no file cache at all, and in either
mode external scans are not cached.
- Cloud clusters are not affected by the BE default, because cloud mode
forces it on. Upgraded clusters keep their session variable value either
way.
**How this PR fixes it**
The first commit makes both defaults true, as in #66684:
`DEFINE_Bool(enable_file_cache, "true")` in `be/src/common/config.cpp`
and `enableFileCache = true` in `SessionVariable`.
On a storage-compute-coupled BE that does not configure the cache, this
turns on the following:
- The cache lives under `${DORIS_HOME}/file_cache` (the default
`file_cache_path`). Without `total_size` it may use the whole file
system. Eviction starts once the file system is 88% full and stops at
85% (`file_cache_enter/exit_need_evict_cache_in_advance_percent`).
`storage_root_path` defaults to the same disk, whose flood stage is 90%,
so a busy cache keeps a shared disk at 85%–88%. To bound it, set
`file_cache_path` with a `total_size`; to keep the old behavior, set
`enable_file_cache = false` in be.conf.
- Only reads through a remote file system go through the cache: external
file scans and cooled-down rowsets. Local rowsets are read exactly as
before.
The second commit keeps out one side effect of the new default.
`SniiCompoundWriter` pads an SNII inverted index container of 32 or more
file cache blocks with zeros, so that it ends on a block boundary. Only
`CachedRemoteFileReader` repays that padding: a read confined to the
last block then fetches one block instead of two. The padding was gated
on `enable_file_cache`, which stood for "cloud mode" as long as the flag
defaulted to false (cloud mode forces it on). With the new default, a
coupled BE would also pad its SNII containers on local disk, by up to
half a block each. There `LocalFileReader` reads them and the padding is
never repaid; branch-4.2 does this today. The padding is now gated on
cloud mode, and its UTs toggle the deploy mode instead of the file
cache.
Two more commits close gaps that the new default makes reachable:
- The third commit refuses to start a BE that has no usable file cache
path. With `ignore_broken_disk=true`,
`FileCacheFactory::create_file_caches` skipped every path that failed to
initialize and still returned OK when all of them were skipped.
`get_by_path(hash)` then took `% 0` and crashed BE at the first cached
read, or earlier at the first rowset cleanup, since
`Rowset::clear_cache()` looks up the cache for every removed rowset
while `enable_file_cache` is on. A coupled BE gets there when
`${DORIS_HOME}/file_cache` cannot be created. `create_file_caches` now
fails in that case, the way storage and spill paths already do ("All
disks are broken, exit.").
- The fourth commit restores the modification time of files listed by
`s3()` / `hdfs()` TVFs. Since #62023,
`ExternalFileTableValuedFunction#parseFile` built `TBrokerFileStatus`
without `FileEntry#modificationTime`, so every TVF range reached BE with
mtime 0. BE keys cached blocks of external files by `path:mtime`, so
with the cache on, an object overwritten in place would keep serving its
old cached blocks. branch-4.x copies the listed mtime
(`BrokerUtil.parseFile`); master does again.
**Results**
| Scenario | Before | After |
|---|---|---|
| New coupled cluster, external table scan | not cached, scan ranges by
round robin | cached, scan ranges by consistent hashing |
| New cloud cluster, external table scan | not cached | cached |
| Coupled cluster upgraded from 4.1.5 / 4.2, be.conf without
`enable_file_cache` | file cache turned off by the upgrade | file cache
stays on |
| Coupled cluster upgraded from 4.1.4 or older | unchanged | BE cache on
(cooled-down rowsets cached). The session variable keeps its persisted
`false`, so external scans stay uncached until `SET GLOBAL
enable_file_cache = true` |
| Internal tables on local disks | unchanged | unchanged |
| SNII index containers, coupled BE | padded only if the BE had turned
the cache on | not padded |
| SNII index containers, cloud mode | padded | padded (unchanged) |
| Every file cache path broken, `ignore_broken_disk=true` | BE starts,
then crashes on first use of the cache | BE refuses to start: "All N
file cache paths are broken" |
| `s3()` / `hdfs()` TVF, object overwritten in place, cache on | could
serve blocks cached from the old content (mtime 0) | the new mtime is
part of the cache key |
Co-authored-by: daidai <[email protected]>
---
be/src/common/config.cpp | 2 +-
be/src/io/cache/block_file_cache_factory.cpp | 6 +++
be/src/runtime/exec_env_init.cpp | 14 ++---
.../index/snii/writer/snii_compound_writer.cpp | 14 ++---
.../index/snii/writer/snii_compound_writer.h | 6 +--
be/test/io/cache/block_file_cache_test.cpp | 44 +++++++++++++++
be/test/io/fs/hdfs_file_system_test.cpp | 3 ++
.../snii/compaction/snii_streamed_session_test.cpp | 19 ++++---
.../snii/writer/snii_compound_writer_test.cpp | 44 ++++++++++-----
.../java/org/apache/doris/qe/SessionVariable.java | 2 +-
.../ExternalFileTableValuedFunction.java | 8 ++-
.../ExternalFileTableValuedFunctionTest.java | 62 ++++++++++++++++++++++
12 files changed, 184 insertions(+), 40 deletions(-)
diff --git a/be/src/common/config.cpp b/be/src/common/config.cpp
index 8419126ae07..dec047775db 100644
--- a/be/src/common/config.cpp
+++ b/be/src/common/config.cpp
@@ -1228,7 +1228,7 @@ DEFINE_Validator(variant_storage_parse_mode,
[](const int config) -> bool { return config >= 0 && config
<= 2; });
// block file cache
-DEFINE_Bool(enable_file_cache, "false");
+DEFINE_Bool(enable_file_cache, "true");
// ATTENTION: For test only. Keep this enabled in production.
// Whether S3 storage write paths populate file cache while writing data to
object storage.
// Disable this for tests that need load and compaction output to bypass file
cache while keeping
diff --git a/be/src/io/cache/block_file_cache_factory.cpp
b/be/src/io/cache/block_file_cache_factory.cpp
index cbf62e77f16..69a64bd3921 100644
--- a/be/src/io/cache/block_file_cache_factory.cpp
+++ b/be/src/io/cache/block_file_cache_factory.cpp
@@ -241,6 +241,12 @@ Status FileCacheFactory::create_file_caches(
return result.status;
}
}
+ // get_by_path() picks an instance by hashing modulo the instance count,
so an empty factory
+ // would crash the first cached read or rowset cleanup. Refuse to start
instead, as doris_main
+ // does when every storage or spill path is broken.
+ if (std::ranges::all_of(results, [](const BuildResult& result) { return
result.skip; })) {
+ return Status::InternalError("All {} file cache paths are broken",
results.size());
+ }
{
std::lock_guard lock(_mtx);
diff --git a/be/src/runtime/exec_env_init.cpp b/be/src/runtime/exec_env_init.cpp
index 4833cbbf5eb..e4230a1b426 100644
--- a/be/src/runtime/exec_env_init.cpp
+++ b/be/src/runtime/exec_env_init.cpp
@@ -511,6 +511,14 @@ void
ExecEnv::init_file_cache_factory(std::vector<doris::CachePath>& cache_paths
}
return;
}
+ // Runs first because it rejects a nonpositive file_cache_each_block_size,
which the
+ // s3_write_buffer_size check below divides by.
+ auto hdfs_batch_status = io::validate_hdfs_write_batch_buffer_size(
+ config::hdfs_write_batch_buffer_size_mb,
config::file_cache_each_block_size);
+ if (!hdfs_batch_status.ok()) {
+ LOG_FATAL("{}", hdfs_batch_status.to_string());
+ exit(-1);
+ }
if (config::file_cache_each_block_size > config::s3_write_buffer_size ||
config::s3_write_buffer_size % config::file_cache_each_block_size !=
0) {
LOG_FATAL(
@@ -520,12 +528,6 @@ void
ExecEnv::init_file_cache_factory(std::vector<doris::CachePath>& cache_paths
config::file_cache_each_block_size,
config::s3_write_buffer_size);
exit(-1);
}
- auto hdfs_batch_status = io::validate_hdfs_write_batch_buffer_size(
- config::hdfs_write_batch_buffer_size_mb,
config::file_cache_each_block_size);
- if (!hdfs_batch_status.ok()) {
- LOG_FATAL("{}", hdfs_batch_status.to_string());
- exit(-1);
- }
Status rest =
doris::parse_conf_cache_paths(doris::config::file_cache_path, cache_paths);
if (!rest) {
throw Exception(
diff --git a/be/src/storage/index/snii/writer/snii_compound_writer.cpp
b/be/src/storage/index/snii/writer/snii_compound_writer.cpp
index 92d817d4d05..54d3ef70be9 100644
--- a/be/src/storage/index/snii/writer/snii_compound_writer.cpp
+++ b/be/src/storage/index/snii/writer/snii_compound_writer.cpp
@@ -22,6 +22,7 @@
#include <algorithm>
#include <utility>
+#include "cloud/config.h"
#include "common/config.h"
#include "storage/index/snii/common/slice.h"
#include "storage/index/snii/encoding/byte_sink.h"
@@ -662,13 +663,14 @@ Status SniiCompoundWriter::write_tail() {
// while the filler bytes stay on disk: strictly worse than never having
padded, until
// compaction rewrites the container.
//
- // Gated on enable_file_cache because the saving is realised only by
CachedRemoteFileReader.
- // That flag defaults to FALSE; without this check a
storage-compute-coupled or local-filesystem
- // deployment appends up to a block of zeros per container and never reads
through a block cache
- // at all. (exec_env_init only validates file_cache_each_block_size when
the cache is on, so in
- // that configuration the value here would also be entirely unvalidated.)
+ // Gated on cloud mode because the saving is realised only by
CachedRemoteFileReader, and only
+ // in cloud mode is every container read through it. A
storage-compute-coupled BE writes its
+ // containers to local disk and reads them through LocalFileReader whether
enable_file_cache is
+ // on (the default) or not, so padding there would append up to a block of
zeros per container
+ // that no block cache ever repays. Cloud mode always runs with the file
cache on, so
+ // exec_env_init has validated file_cache_each_block_size before it is
read here.
const int64_t block = config::file_cache_each_block_size;
- if (config::enable_file_cache && block > 0) {
+ if (config::is_cloud_mode() && block > 0) {
const uint64_t unpadded = out_->bytes_written() +
tail_sink.buffer().size();
const auto block_size = static_cast<uint64_t>(block);
const uint64_t pad = (block_size - unpadded % block_size) % block_size;
diff --git a/be/src/storage/index/snii/writer/snii_compound_writer.h
b/be/src/storage/index/snii/writer/snii_compound_writer.h
index 573a952a6df..66f1d962d92 100644
--- a/be/src/storage/index/snii/writer/snii_compound_writer.h
+++ b/be/src/storage/index/snii/writer/snii_compound_writer.h
@@ -55,9 +55,9 @@ class SniiRewriteSnapshot;
// for each logical index, in add order:
// [Core metadata][SampledTermIndex blob][DICT block directory blob]
// [metadata directory] raw SniiMetadataDirectoryPB bytes
-// [block padding] OPTIONAL run of zero bytes, see write_tail().
Written only for
-// containers of >= kMinPaddingLeverage cache
blocks, and only when the
-// file cache is on, so that the container ends on
a block boundary.
+// [block padding] OPTIONAL run of zero bytes, see write_tail().
Written only in cloud
+// mode and only for containers of >=
kMinPaddingLeverage cache blocks,
+// so that the container ends on a block boundary.
// Referenced by nothing and read by nobody -- but
it means the metadata
// directory is NOT necessarily adjacent to the
tail pointer, and that
// container length is not a pure function of the
indexed content (two
diff --git a/be/test/io/cache/block_file_cache_test.cpp
b/be/test/io/cache/block_file_cache_test.cpp
index a84288f4521..85935068f9c 100644
--- a/be/test/io/cache/block_file_cache_test.cpp
+++ b/be/test/io/cache/block_file_cache_test.cpp
@@ -4260,6 +4260,50 @@ TEST_F(BlockFileCacheTest,
create_file_caches_preserves_config_order) {
}
}
+TEST_F(BlockFileCacheTest,
create_file_caches_rejects_a_factory_left_without_any_cache) {
+ reset_file_cache_factory_for_test();
+ // A regular file where a cache directory would go: no cache can be
created below it.
+ const std::string blocker = (caches_dir / "cache_path_blocker").string();
+ const std::string broken_path = (caches_dir / "cache_path_blocker" /
"cache" / "").string();
+ const std::string usable_path = (caches_dir / "usable_cache" /
"").string();
+ fs::remove_all(blocker);
+ fs::remove_all(usable_path);
+ std::ofstream(blocker).close();
+ Defer cleanup {[&] {
+ reset_file_cache_factory_for_test();
+ fs::remove_all(blocker);
+ fs::remove_all(usable_path);
+ }};
+ auto cache_path = [](const std::string& path) {
+ return CachePath(path, 90, 30, DEFAULT_NORMAL_PERCENT,
DEFAULT_DISPOSABLE_PERCENT,
+ DEFAULT_INDEX_PERCENT, DEFAULT_TTL_PERCENT, "disk");
+ };
+ std::vector<std::string> skipped;
+ auto skip_broken = [&skipped](const std::string& path, const Status&) {
+ skipped.push_back(path);
+ return true;
+ };
+
+ // ignore_broken_disk skips the only path. get_by_path() would then divide
by zero, so the
+ // factory must report failure rather than an empty success.
+ EXPECT_FALSE(FileCacheFactory::instance()
+ ->create_file_caches({cache_path(broken_path)},
skip_broken)
+ .ok());
+ EXPECT_EQ(skipped, std::vector<std::string> {broken_path});
+ EXPECT_EQ(FileCacheFactory::instance()->get_cache_instance_size(), 0);
+
+ // With a usable path left, the broken one is still skipped and the usable
one is kept.
+ skipped.clear();
+ ASSERT_TRUE(FileCacheFactory::instance()
+ ->create_file_caches({cache_path(broken_path),
cache_path(usable_path)},
+ skip_broken)
+ .ok());
+ EXPECT_EQ(skipped, std::vector<std::string> {broken_path});
+ ASSERT_EQ(FileCacheFactory::instance()->get_cache_instance_size(), 1);
+ EXPECT_EQ(FileCacheFactory::instance()->get_caches()[0]->get_base_path(),
usable_path);
+ wait_until_cache_ready(*FileCacheFactory::instance()->get_caches()[0]);
+}
+
TEST_F(BlockFileCacheTest, test_factory_2) {
if (fs::exists(cache_base_path)) {
fs::remove_all(cache_base_path);
diff --git a/be/test/io/fs/hdfs_file_system_test.cpp
b/be/test/io/fs/hdfs_file_system_test.cpp
index 9860cc8c1ee..859b728fcdd 100644
--- a/be/test/io/fs/hdfs_file_system_test.cpp
+++ b/be/test/io/fs/hdfs_file_system_test.cpp
@@ -40,6 +40,9 @@ TEST(HdfsFileSystemTest,
RejectsCacheBlockSizeNotDividingHdfsBatch) {
EXPECT_TRUE(io::validate_hdfs_write_batch_buffer_size(1, 256 * 1024).ok());
EXPECT_FALSE(io::validate_hdfs_write_batch_buffer_size(1, 640 *
1024).ok());
EXPECT_FALSE(io::validate_hdfs_write_batch_buffer_size(1, 2 * 1024 *
1024).ok());
+ // ExecEnv::init_file_cache_factory relies on this rejection before it
divides by the size.
+ EXPECT_FALSE(io::validate_hdfs_write_batch_buffer_size(1, 0).ok());
+ EXPECT_FALSE(io::validate_hdfs_write_batch_buffer_size(1, -1).ok());
}
TEST(HdfsFileSystemTest, Write) {
diff --git
a/be/test/storage/index/snii/compaction/snii_streamed_session_test.cpp
b/be/test/storage/index/snii/compaction/snii_streamed_session_test.cpp
index 71708f5436c..170e00de740 100644
--- a/be/test/storage/index/snii/compaction/snii_streamed_session_test.cpp
+++ b/be/test/storage/index/snii/compaction/snii_streamed_session_test.cpp
@@ -29,6 +29,7 @@
#include <utility>
#include <vector>
+#include "cloud/config.h"
#include "common/config.h"
#include "common/status.h"
#include "storage/index/snii/common/slice.h"
@@ -396,16 +397,20 @@ Status
begin_scoring_session_from_local_input(SniiCompoundWriter* compound,
// compaction and load produce different images for the same input.
TEST(SniiStreamedWriterSessionTest,
StreamedContainerPadsIdenticallyToOrdinary) {
const int64_t saved_block = doris::config::file_cache_each_block_size;
- const bool saved_cache = doris::config::enable_file_cache;
+ const std::string saved_deploy_mode = doris::config::deploy_mode;
+ const std::string saved_cloud_unique_id = doris::config::cloud_unique_id;
doris::Defer restore {[&] {
doris::config::file_cache_each_block_size = saved_block;
- doris::config::enable_file_cache = saved_cache;
+ doris::config::deploy_mode = saved_deploy_mode;
+ doris::config::cloud_unique_id = saved_cloud_unique_id;
}};
+ doris::config::cloud_unique_id.clear();
- // Measure the unpadded size first, then pick a block the gate must
accept: the container
- // spans at least kMinPaddingLeverage blocks (we start the search at 2x
that leverage and walk
- // the block size up), and pad is asserted to be both due and worth paying.
- doris::config::enable_file_cache = false;
+ // Measure the unpadded size first, outside cloud mode where nothing is
padded, then pick a
+ // block the gate must accept: the container spans at least
kMinPaddingLeverage blocks (we start
+ // the search at 2x that leverage and walk the block size up), and pad is
asserted to be both due
+ // and worth paying.
+ doris::config::deploy_mode = "";
MemoryFile unpadded_file;
assert_ok(write_streamed_index(representative_input(), &unpadded_file));
const size_t unpadded = unpadded_file.data().size();
@@ -425,7 +430,7 @@ TEST(SniiStreamedWriterSessionTest,
StreamedContainerPadsIdenticallyToOrdinary)
ASSERT_LT(2 * pad, block_size) << "the gate would decline this padding as
not worth its cost";
const auto block = static_cast<int64_t>(block_size);
- doris::config::enable_file_cache = true;
+ doris::config::deploy_mode = "cloud";
doris::config::file_cache_each_block_size = block;
MemoryFile ordinary;
MemoryFile streamed;
diff --git a/be/test/storage/index/snii/writer/snii_compound_writer_test.cpp
b/be/test/storage/index/snii/writer/snii_compound_writer_test.cpp
index 1fd58df5042..dbee49f7b35 100644
--- a/be/test/storage/index/snii/writer/snii_compound_writer_test.cpp
+++ b/be/test/storage/index/snii/writer/snii_compound_writer_test.cpp
@@ -30,6 +30,7 @@
#include <utility>
#include <vector>
+#include "cloud/config.h"
#include "common/config.h"
#include "common/status.h"
#include "gen_cpp/snii.pb.h"
@@ -1491,16 +1492,24 @@ TEST(SniiCompoundWriter,
PacksEachIndexSectionsContiguouslyWithoutInterleaving)
namespace {
// Writes the standard two-index fixture at the given cache block size and
returns the bytes.
-// The file cache is forced on by default: padding is gated on it and it
defaults to OFF, so a
-// fixture that left it alone would exercise only the not-padded branch
whatever block size it picks.
-std::vector<uint8_t> WriteFixtureAtBlockSize(int64_t block_size, bool
file_cache_on = true) {
+// Cloud mode is forced on by default: padding is gated on it and a UT process
is not in cloud
+// mode, so a fixture that left it alone would exercise only the not-padded
branch whatever block
+// size it picks. The file cache is on either way, as it is by default in both
deploy modes, so the
+// deploy mode is the only thing that decides.
+std::vector<uint8_t> WriteFixtureAtBlockSize(int64_t block_size, bool
cloud_mode = true) {
const int64_t saved_block = doris::config::file_cache_each_block_size;
const bool saved_cache = doris::config::enable_file_cache;
+ const std::string saved_deploy_mode = doris::config::deploy_mode;
+ const std::string saved_cloud_unique_id = doris::config::cloud_unique_id;
doris::config::file_cache_each_block_size = block_size;
- doris::config::enable_file_cache = file_cache_on;
+ doris::config::enable_file_cache = true;
+ doris::config::deploy_mode = cloud_mode ? "cloud" : "";
+ doris::config::cloud_unique_id.clear();
doris::Defer restore {[&] {
doris::config::file_cache_each_block_size = saved_block;
doris::config::enable_file_cache = saved_cache;
+ doris::config::deploy_mode = saved_deploy_mode;
+ doris::config::cloud_unique_id = saved_cloud_unique_id;
}};
const std::string path = TempPath();
@@ -1614,19 +1623,20 @@ TEST(SniiCompoundWriter,
SkipsPaddingWhenItWouldBeLargeRelativeToContainer) {
ExpectFixtureReadable(file);
}
-// Padding is repaid only by CachedRemoteFileReader's block alignment.
enable_file_cache defaults
-// to OFF, and with it off there is no block cache in the read path at all --
so a container that
-// qualifies on every other count must still come out unpadded.
-TEST(SniiCompoundWriter, SkipsPaddingWhenTheFileCacheIsOff) {
+// Padding is repaid only by CachedRemoteFileReader's block alignment, and
only cloud mode reads
+// containers through it. A storage-compute-coupled BE reads them from local
disk even with the
+// file cache on, which it is by default -- so outside cloud mode a container
that qualifies on
+// every other count must still come out unpadded.
+TEST(SniiCompoundWriter, SkipsPaddingOutsideCloudMode) {
const size_t unpadded = WriteFixtureAtBlockSize(0).size();
const auto block = CheapPaddingBlockSize(unpadded);
ASSERT_GE(block, 2) << "fixture too small to derive a usable block size";
- // Same block size the acceptance test uses, so the cache flag is the only
difference.
- ASSERT_NE(WriteFixtureAtBlockSize(block, /*file_cache_on=*/true).size(),
unpadded)
- << "precondition: this block size must pad when the cache is on";
+ // Same block size the acceptance test uses, so the deploy mode is the
only difference.
+ ASSERT_NE(WriteFixtureAtBlockSize(block, /*cloud_mode=*/true).size(),
unpadded)
+ << "precondition: this block size must pad in cloud mode";
- const std::vector<uint8_t> file = WriteFixtureAtBlockSize(block,
/*file_cache_on=*/false);
- EXPECT_EQ(file.size(), unpadded) << "padded despite there being no block
cache to repay it";
+ const std::vector<uint8_t> file = WriteFixtureAtBlockSize(block,
/*cloud_mode=*/false);
+ EXPECT_EQ(file.size(), unpadded) << "padded outside cloud mode, where no
block cache repays it";
ExpectFixtureReadable(file);
}
@@ -1634,8 +1644,14 @@ TEST(SniiCompoundWriter,
SkipsPaddingWhenTheFileCacheIsOff) {
// consult it at all, rather than treating 0 as "align to everything".
TEST(SniiCompoundWriter, AddsNoPaddingWhenBlockSizeIsDisabled) {
const int64_t saved_block = doris::config::file_cache_each_block_size;
+ const std::string saved_deploy_mode = doris::config::deploy_mode;
doris::config::file_cache_each_block_size = 0;
- doris::Defer restore {[&] { doris::config::file_cache_each_block_size =
saved_block; }};
+ // Cloud mode, so that the disabled block size is the only thing that can
turn padding off.
+ doris::config::deploy_mode = "cloud";
+ doris::Defer restore {[&] {
+ doris::config::file_cache_each_block_size = saved_block;
+ doris::config::deploy_mode = saved_deploy_mode;
+ }};
const std::string path = TempPath();
{
diff --git a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
index bc086c7596e..1cdfc01674a 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java
@@ -2363,7 +2363,7 @@ public class SessionVariable implements Serializable,
Writable {
@VarAttrDef.VarAttr(name = ENABLE_FILE_CACHE, needForward = true,
description = "Set wether to use file cache. "
+ "This variable takes effect only if the BE config
enable_file_cache=true. "
+ "The cache is not used when BE config enable_file_cache=false.")
- public boolean enableFileCache = false;
+ public boolean enableFileCache = true;
// Specify base path for file cache, or chose a random path.
@VarAttrDef.VarAttr(name = FILE_CACHE_BASE_PATH, needForward = true,
description = "Specify the storage path of "
diff --git
a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java
b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java
index 846818ecb65..2b6fb104493 100644
---
a/fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java
+++
b/fe/fe-core/src/main/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunction.java
@@ -184,8 +184,12 @@ public abstract class ExternalFileTableValuedFunction
extends TableValuedFunctio
entries = fs.listFiles(Location.of(path));
}
for (FileEntry e : entries) {
- fileStatuses.add(new TBrokerFileStatus(
- e.location().uri(), e.isDirectory(), e.length(),
!e.isDirectory()));
+ TBrokerFileStatus status = new TBrokerFileStatus(
+ e.location().uri(), e.isDirectory(), e.length(),
!e.isDirectory());
+ // BE keys cached blocks of external files by path and
modification time, so a
+ // file overwritten in place must arrive with its new
mtime, not as mtime 0.
+ status.setModificationTime(e.modificationTime());
+ fileStatuses.add(status);
}
} catch (IOException e) {
throw new UserException("list files failed for path " + path +
": " + e.getMessage(), e);
diff --git
a/fe/fe-core/src/test/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunctionTest.java
b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunctionTest.java
index e3459200187..92b024c2fb8 100644
---
a/fe/fe-core/src/test/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunctionTest.java
+++
b/fe/fe-core/src/test/java/org/apache/doris/tablefunction/ExternalFileTableValuedFunctionTest.java
@@ -17,6 +17,7 @@
package org.apache.doris.tablefunction;
+import org.apache.doris.analysis.BrokerDesc;
import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.PrimitiveType;
import org.apache.doris.catalog.StructField;
@@ -27,9 +28,16 @@ import org.apache.doris.common.Config;
import org.apache.doris.common.Pair;
import org.apache.doris.common.util.FileFormatConstants;
import org.apache.doris.common.util.FileFormatUtils;
+import org.apache.doris.filesystem.FileEntry;
+import org.apache.doris.filesystem.FileSystem;
+import org.apache.doris.filesystem.GlobListing;
+import org.apache.doris.filesystem.Location;
+import org.apache.doris.fs.FileSystemFactory;
import org.apache.doris.proto.Types.PScalarType;
import org.apache.doris.proto.Types.PStructField;
import org.apache.doris.proto.Types.PTypeNode;
+import org.apache.doris.thrift.TBrokerFileStatus;
+import org.apache.doris.thrift.TFileType;
import org.apache.doris.thrift.TPrimitiveType;
import org.apache.doris.thrift.TTypeNodeType;
@@ -37,6 +45,7 @@ import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
import org.mockito.Mockito;
import java.lang.reflect.Method;
@@ -45,6 +54,59 @@ import java.util.List;
import java.util.Map;
public class ExternalFileTableValuedFunctionTest {
+ @Test
+ public void testListedFilesCarryTheirModificationTime() throws Exception {
+ // BE keys cached blocks of an external file by path and modification
time. Listing files
+ // without their mtime ships every TVF range as mtime 0, so an object
overwritten in place
+ // would keep serving the blocks cached from its old content.
+ FileSystem fs = Mockito.mock(FileSystem.class);
+ Mockito.when(fs.globListWithLimit(Mockito.any(Location.class),
Mockito.anyString(),
+ Mockito.anyLong(), Mockito.anyLong()))
+ .thenReturn(new GlobListing(Arrays.asList(
+ new FileEntry(Location.of("s3://bucket/dir/a.csv"),
10L, false, 1_700_000_000_123L, null),
+ new FileEntry(Location.of("s3://bucket/dir/b.csv"),
20L, false, 1_700_000_000_456L, null)),
+ "bucket", "dir/", "dir/b.csv"));
+ BrokerDesc brokerDesc = Mockito.mock(BrokerDesc.class);
+ ListingTvf tvf = new ListingTvf(brokerDesc);
+ try (MockedStatic<FileSystemFactory> factory =
Mockito.mockStatic(FileSystemFactory.class)) {
+ factory.when(() ->
FileSystemFactory.getFileSystem(brokerDesc)).thenReturn(fs);
+ tvf.parseFile();
+ }
+
+ List<TBrokerFileStatus> statuses = tvf.getFileStatuses();
+ Assertions.assertEquals(2, statuses.size());
+ Assertions.assertEquals(1_700_000_000_123L,
statuses.get(0).getModificationTime());
+ Assertions.assertEquals(1_700_000_000_456L,
statuses.get(1).getModificationTime());
+ }
+
+ private static final class ListingTvf extends
ExternalFileTableValuedFunction {
+ private final BrokerDesc brokerDesc;
+
+ private ListingTvf(BrokerDesc brokerDesc) {
+ this.brokerDesc = brokerDesc;
+ }
+
+ @Override
+ public TFileType getTFileType() {
+ return TFileType.FILE_S3;
+ }
+
+ @Override
+ public String getFilePath() {
+ return "s3://bucket/dir/*.csv";
+ }
+
+ @Override
+ public BrokerDesc getBrokerDesc() {
+ return brokerDesc;
+ }
+
+ @Override
+ public String getTableName() {
+ return "ListingTvf";
+ }
+ }
+
@Test
public void testFileSchemaPreservesNestedFieldSpelling() throws Exception {
ExternalFileTableValuedFunction tvf = Mockito.mock(
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
