github-actions[bot] commented on code in PR #68797:
URL: https://github.com/apache/doris/pull/68797#discussion_r4226517611
##########
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:
[P1] Avoid caching mutable TVF files without a versioned key. HTTP and
generic file TVFs create `TBrokerFileStatus` without `modificationTime`, so
their ranges reach BE with mtime 0. This new session default makes
`FileFactory` cache them, while `CachedRemoteFileReader` hashes only
`path:mtime`. If a stable HTTP URL or object key is overwritten with different
same-size rows, the next query reuses `path:0` blocks and returns stale rows.
Supply a reliable version for the cache key or keep caching off for these
unversioned ranges.
##########
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:
[P1] Ensure the default cache has a usable directory before enabling it. A
coupled BE can have writable `storage_root_path=/data/doris` but read-only
`${DORIS_HOME}`; the default cache path `${DORIS_HOME}/file_cache` then fails
creation and this new true default makes BE startup fail. With
`ignore_broken_disk=true`, the failed sole cache is skipped and startup
succeeds with `_caches` empty; the first cached read or `Rowset::clear_cache()`
calls `get_by_path(hash)`, which takes `% _caches.size()` and crashes. Please
provide a usable default/fallback and reject or disable caching when every path
is skipped.
##########
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:
[P1] Namespace external cache entries by the actual source. Two S3 TVFs can
read the same `s3://bucket/key` from different `s3.endpoint` values, or two
HTTP TVFs can request different equal-length representations of one URL using
different headers. Their TVF mtime is 0, and the new default sends both through
a cache keyed only by `path:mtime`; the second query on the same BE can return
the first source's rows even when both files are immutable. Include a stable
endpoint/request-representation identity in the key or bypass shared caching
for these sources.
##########
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:
[P2] Preserve tail alignment for rowsets that will be cooled to remote
storage. A coupled tablet with a storage policy writes its SNII index locally,
then `BetaRowset::upload_to()` copies that file unchanged to remote storage.
Its later `IndexFileReader` uses `CachedRemoteFileReader`, whose EOF read
back-pads a partial final block by one whole cache block. This cloud-only gate
suppresses padding for those eligible local writes, losing the previous
cold-read saving. Account for cooldown when choosing the gate or retain
alignment for remotely cached rowsets.
--
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]
