github-actions[bot] commented on code in PR #64961:
URL: https://github.com/apache/doris/pull/64961#discussion_r3568416685
##########
be/src/io/fs/file_meta_cache.cpp:
##########
@@ -40,4 +59,95 @@ std::string FileMetaCache::get_key(io::FileReaderSPtr
file_reader,
_file_description.file_size == -1 ? file_reader->size() :
_file_description.file_size);
}
+bool FileMetaCache::is_persistent_cache_enabled() {
+ return config::enable_external_file_meta_disk_cache &&
+ config::external_file_meta_disk_cache_max_entry_bytes > 0;
+}
+
+bool FileMetaCache::is_persistent_cache_payload_size_allowed(uint64_t
payload_size) {
+ const int64_t max_entry_bytes =
config::external_file_meta_disk_cache_max_entry_bytes;
+ return config::enable_external_file_meta_disk_cache && max_entry_bytes > 0
&&
+ payload_size <= static_cast<uint64_t>(max_entry_bytes);
+}
+
+FileMetaCacheLookupResult FileMetaCache::lookup(const FileMetaCacheContext&
context,
+ ObjLRUCache::CacheHandle*
handle,
+ std::string* serialized_meta,
+ FileMetaCacheProfile* profile)
{
+ DCHECK(handle != nullptr);
+ DCHECK(serialized_meta != nullptr);
+ if (context.enable_memory_cache && lookup(context.key, handle)) {
+ serialized_meta->clear();
Review Comment:
The context lookup path can return `PERSISTED_HIT` or `MISS` while leaving
`handle` untouched. `ObjLRUCache::lookup()` returns false without assigning the
output handle, so a caller that reuses a handle from a previous memory hit
keeps that unrelated entry pinned, and `handle.valid()` still points at stale
metadata even though this result did not produce a memory-cache object. Please
clear the output handle before non-memory outcomes, or at function entry, and
add a reuse test that first obtains a memory handle and then gets a persistent
hit or miss with the same handle.
##########
be/src/io/fs/file_meta_cache.cpp:
##########
@@ -40,4 +59,95 @@ std::string FileMetaCache::get_key(io::FileReaderSPtr
file_reader,
_file_description.file_size == -1 ? file_reader->size() :
_file_description.file_size);
}
+bool FileMetaCache::is_persistent_cache_enabled() {
+ return config::enable_external_file_meta_disk_cache &&
+ config::external_file_meta_disk_cache_max_entry_bytes > 0;
+}
+
+bool FileMetaCache::is_persistent_cache_payload_size_allowed(uint64_t
payload_size) {
+ const int64_t max_entry_bytes =
config::external_file_meta_disk_cache_max_entry_bytes;
+ return config::enable_external_file_meta_disk_cache && max_entry_bytes > 0
&&
+ payload_size <= static_cast<uint64_t>(max_entry_bytes);
+}
+
+FileMetaCacheLookupResult FileMetaCache::lookup(const FileMetaCacheContext&
context,
+ ObjLRUCache::CacheHandle*
handle,
+ std::string* serialized_meta,
+ FileMetaCacheProfile* profile)
{
+ DCHECK(handle != nullptr);
+ DCHECK(serialized_meta != nullptr);
+ if (context.enable_memory_cache && lookup(context.key, handle)) {
+ serialized_meta->clear();
+ if (profile != nullptr) {
+ update_profile_counter(profile->hit_cache);
+ update_profile_counter(profile->hit_memory_cache);
+ }
+ return {.state = FileMetaCacheLookupState::MEMORY_HIT};
+ }
+
+ FileMetaCacheLookupResult result;
+ int64_t persisted_read_time = 0;
+ if (lookup_persistent_cache(context, serialized_meta,
&persisted_read_time)) {
+ result.state = FileMetaCacheLookupState::PERSISTED_HIT;
+ if (profile != nullptr) {
+ update_profile_counter(profile->hit_cache);
+ update_profile_counter(profile->hit_disk_cache);
+ update_profile_counter(profile->read_disk_cache_time,
persisted_read_time);
+ }
+ } else if (is_persistent_cache_enabled() && profile != nullptr) {
+ update_profile_counter(profile->miss_disk_cache);
+ }
+ return result;
+}
+
+void FileMetaCache::invalidate_persistent_cache(const FileMetaCacheContext&
context) {
+ if (_persistent_cache != nullptr) {
+ _persistent_cache->remove(context.format, context.key);
+ }
+}
+
+bool FileMetaCache::lookup_persistent_cache(const FileMetaCacheContext&
context,
+ std::string* payload, int64_t*
read_time) {
+ DCHECK(payload != nullptr);
+ DCHECK(read_time != nullptr);
+ payload->clear();
+ *read_time = 0;
+ if (!is_persistent_cache_enabled() || _persistent_cache == nullptr) {
+ return false;
+ }
+
+ MonotonicStopWatch watch;
+ watch.start();
+ Status status = _persistent_cache->read(context.format, context.key,
context.modification_time,
+ context.file_size, payload);
Review Comment:
This read path bypasses the max-entry-size gate that the write path
enforces. Since `external_file_meta_disk_cache_max_entry_bytes` is mutable,
lowering it after entries were written, or reading an old/corrupt backend
entry, can still return an oversized payload as `PERSISTED_HIT`. Please check
`payload->size()` after a successful read and treat oversized payloads as a
miss, optionally invalidating them, or make `FileMetaPersistentCache::read()`
explicitly enforce the configured limit and cover oversized reads in the unit
test.
--
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]
