morningman commented on code in PR #68797:
URL: https://github.com/apache/doris/pull/68797#discussion_r4226698613
##########
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");
Review Comment:
Thanks, two parts:
- **Empty factory**: fixed in 747dd5487de. `create_file_caches` now fails
when every configured path was skipped, so startup stops with "failed to init
file cache ... All N file cache paths are broken", the same way `doris_main`
handles storage and spill paths ("All disks are broken, exit."). The crash
would in fact come before the first cached read: `Rowset::clear_cache()` looks
up the cache for every removed rowset while `enable_file_cache` is on. Covered
by the new
`BlockFileCacheTest.create_file_caches_rejects_a_factory_left_without_any_cache`.
- **Unusable default directory**: failing to start is intended. branch-4.1
(from 4.1.5) and branch-4.2 already enable the cache by default with the same
path and the same behavior. Silently falling back to another disk would put
cache data where the operator did not ask for it. The release note now states
that BE refuses to start when `${DORIS_HOME}/file_cache` cannot be created, and
how to set `file_cache_path` or `enable_file_cache = false`.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -2352,7 +2352,7 @@ public boolean isEnableHboNonStrictMatchingMode() {
@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;
Review Comment:
Confirmed for the generic file TVFs, fixed in 0911a803d4a. Since #62023
moved TVF listing onto the filesystem SPI, `parseFile` dropped
`FileEntry#modificationTime` when building `TBrokerFileStatus`, so `s3()` /
`hdfs()` ranges reached BE with mtime 0. They now carry the listed modification
time again, as on branch-4.x (`BrokerUtil.parseFile` copies it). Covered by
`ExternalFileTableValuedFunctionTest#testListedFilesCarryTheirModificationTime`.
`http()` has never carried a version on any branch, including 4.1.5 / 4.2
where the cache is already on by default. Making it safe needs a validator from
the response (`Last-Modified` / `ETag`), or bypassing the cache when there is
none. That is a change to the HTTP TVF itself, so it will be tracked separately
rather than in this default-value PR.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -2352,7 +2352,7 @@ public boolean isEnableHboNonStrictMatchingMode() {
@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;
Review Comment:
The cache key layout (`path:mtime`, with no endpoint or request identity) is
the existing design for all external reads, on master and branch-4.x alike;
this PR only changes the defaults. The main exposure described here, TVF ranges
with mtime 0, is removed by 0911a803d4a: two S3 / HDFS sources now collide only
if they share both the path and the modification time. Adding the source
identity (endpoint / filesystem identity, and the HTTP request representation)
to the external cache key belongs in the file cache layer and invalidates
existing cache entries, so it will be tracked as a separate change.
##########
be/src/storage/index/snii/writer/snii_compound_writer.cpp:
##########
@@ -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) {
Review Comment:
This is a deliberate trade-off, and we keep the cloud-mode gate:
- Most SNII containers on a storage-compute-coupled BE are never cooled
down. Padding all of them, which is what branch-4.2 does since it gates on
`enable_file_cache` (now true by default), leaves up to half a cache block of
zeros per container on local disk until compaction rewrites it.
- Without padding, a container of 32 or more blocks that does cool down
costs one extra cache block (1 MiB by default) on its first cold tail read;
after that the block is cached.
- Gating on the tablet's storage policy would mean plumbing that information
through `IndexFileWriter` into `SniiCompoundWriter`, and it would still miss
policies added after the data was written.
A cooldown-aware gate can be a follow-up if the cold-read cost shows up in
practice.
--
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]
