From e7fff74797c97e0b1e9325726911513c7d201ccd Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 8 May 2023 17:45:42 +0000 Subject: [PATCH 1/2] Cleanup --- src/Interpreters/Cache/FileSegment.h | 2 +- src/Interpreters/Cache/Guards.h | 22 ++++------------------ src/Interpreters/Cache/Metadata.cpp | 14 +++----------- 3 files changed, 8 insertions(+), 30 deletions(-) diff --git a/src/Interpreters/Cache/FileSegment.h b/src/Interpreters/Cache/FileSegment.h index 60883631177..163a15fcfda 100644 --- a/src/Interpreters/Cache/FileSegment.h +++ b/src/Interpreters/Cache/FileSegment.h @@ -61,7 +61,7 @@ struct CreateFileSegmentSettings : kind(kind_), unbounded(unbounded_) {} }; -class FileSegment : private boost::noncopyable, public std::enable_shared_from_this +class FileSegment : private boost::noncopyable { friend struct LockedKey; friend class FileCache; /// Because of reserved_size in tryReserve(). diff --git a/src/Interpreters/Cache/Guards.h b/src/Interpreters/Cache/Guards.h index 0e06495bd82..66382d82003 100644 --- a/src/Interpreters/Cache/Guards.h +++ b/src/Interpreters/Cache/Guards.h @@ -1,8 +1,6 @@ #pragma once #include -#include #include -#include namespace DB { @@ -63,10 +61,7 @@ namespace DB */ struct CacheGuard : private boost::noncopyable { - struct Lock : public std::unique_lock - { - explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} - }; + using Lock = std::unique_lock; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -77,10 +72,7 @@ struct CacheGuard : private boost::noncopyable */ struct CacheMetadataGuard : private boost::noncopyable { - struct Lock : public std::unique_lock - { - explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} - }; + using Lock = std::unique_lock; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -91,10 +83,7 @@ struct CacheMetadataGuard : private boost::noncopyable */ struct KeyGuard : private boost::noncopyable { - struct Lock : public std::unique_lock - { - explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} - }; + using Lock = std::unique_lock; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -105,10 +94,7 @@ struct KeyGuard : private boost::noncopyable */ struct FileSegmentGuard : private boost::noncopyable { - struct Lock : public std::unique_lock - { - explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} - }; + using Lock = std::unique_lock; Lock lock() { return Lock(mutex); } std::mutex mutex; diff --git a/src/Interpreters/Cache/Metadata.cpp b/src/Interpreters/Cache/Metadata.cpp index 02ec7fdbefa..1aa13d4e98c 100644 --- a/src/Interpreters/Cache/Metadata.cpp +++ b/src/Interpreters/Cache/Metadata.cpp @@ -208,10 +208,9 @@ LockedKeyPtr CacheMetadata::lockKeyMetadata( chassert(key_not_found_policy == KeyNotFoundPolicy::CREATE_EMPTY); } - /// Not we are at a case: - /// key_state == KeyMetadata::KeyState::REMOVED - /// and KeyNotFoundPolicy == CREATE_EMPTY - /// Retry. + /// Now we are at the case when the key was removed (key_state == KeyMetadata::KeyState::REMOVED) + /// but we need to return empty key (key_not_found_policy == KeyNotFoundPolicy::CREATE_EMPTY) + /// Retry return lockKeyMetadata(key, key_not_found_policy); } @@ -241,13 +240,6 @@ void CacheMetadata::doCleanup() { auto lock = guard.lock(); - /// Let's mention this case. - /// This metadata cleanup is delayed so what is we marked key as deleted and - /// put it to deletion queue, but then the same key was added to cache before - /// we actually performed this delayed removal? - /// In this case it will work fine because on each attempt to add any key to cache - /// we perform this delayed removal. - FileCacheKey cleanup_key; while (cleanup_queue->tryPop(cleanup_key)) { From 903021e896e88f8c5c144caf3de55df1cd523a0f Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 8 May 2023 18:29:47 +0000 Subject: [PATCH 2/2] Fix review comment --- src/Interpreters/Cache/Guards.h | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/Cache/Guards.h b/src/Interpreters/Cache/Guards.h index 66382d82003..09586b55c61 100644 --- a/src/Interpreters/Cache/Guards.h +++ b/src/Interpreters/Cache/Guards.h @@ -61,7 +61,12 @@ namespace DB */ struct CacheGuard : private boost::noncopyable { - using Lock = std::unique_lock; + /// struct is used (not keyword `using`) to make CacheGuard::Lock non-interchangable with other guards locks + /// so, we wouldn't be able to pass CacheGuard::Lock to a function which accepts KeyGuard::Lock, for example + struct Lock : public std::unique_lock + { + explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} + }; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -72,7 +77,10 @@ struct CacheGuard : private boost::noncopyable */ struct CacheMetadataGuard : private boost::noncopyable { - using Lock = std::unique_lock; + struct Lock : public std::unique_lock + { + explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} + }; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -83,7 +91,10 @@ struct CacheMetadataGuard : private boost::noncopyable */ struct KeyGuard : private boost::noncopyable { - using Lock = std::unique_lock; + struct Lock : public std::unique_lock + { + explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} + }; Lock lock() { return Lock(mutex); } std::mutex mutex; @@ -94,7 +105,10 @@ struct KeyGuard : private boost::noncopyable */ struct FileSegmentGuard : private boost::noncopyable { - using Lock = std::unique_lock; + struct Lock : public std::unique_lock + { + explicit Lock(std::mutex & mutex_) : std::unique_lock(mutex_) {} + }; Lock lock() { return Lock(mutex); } std::mutex mutex;