From fd7c732c120585478cdd8300445bdba4f0394276 Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 19 Mar 2024 23:01:03 +0100 Subject: [PATCH] Add one more comment, better cache policy randomizer --- docker/test/stateful/run.sh | 14 +++++++------- src/Interpreters/Cache/FileCache.cpp | 2 +- src/Interpreters/Cache/IFileCachePriority.h | 9 +++++++++ src/Interpreters/Cache/SLRUFileCachePriority.cpp | 5 ----- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/docker/test/stateful/run.sh b/docker/test/stateful/run.sh index 18ac909df54..c2e9fdfe41d 100755 --- a/docker/test/stateful/run.sh +++ b/docker/test/stateful/run.sh @@ -25,7 +25,7 @@ azurite-blob --blobHost 0.0.0.0 --blobPort 10000 --debug /azurite_log & config_logs_export_cluster /etc/clickhouse-server/config.d/system_logs_export.yaml cache_policy="" -if [ $(( $(date +%-d) % 2 )) -eq 1 ]; then +if [ $(($RANDOM%2)) -eq 1 ]; then cache_policy="SLRU" else cache_policy="LRU" @@ -33,12 +33,12 @@ fi echo "Using cache policy: $cache_policy" -#if [ "$cache_policy" = "SLRU" ]; then -# sudo cat /etc/clickhouse-server/config.d/storage_conf.xml \ -# | sed "s|LRU|SLRU|" \ -# > /etc/clickhouse-server/config.d/storage_conf.xml.tmp -# mv /etc/clickhouse-server/config.d/storage_conf.xml.tmp /etc/clickhouse-server/config.d/storage_conf.xml -#fi +if [ "$cache_policy" = "SLRU" ]; then + sudo cat /etc/clickhouse-server/config.d/storage_conf.xml \ + | sed "s|LRU|SLRU|" \ + > /etc/clickhouse-server/config.d/storage_conf.xml.tmp + mv /etc/clickhouse-server/config.d/storage_conf.xml.tmp /etc/clickhouse-server/config.d/storage_conf.xml +fi if [[ -n "$USE_S3_STORAGE_FOR_MERGE_TREE" ]] && [[ "$USE_S3_STORAGE_FOR_MERGE_TREE" -eq 1 ]]; then # It is not needed, we will explicitly create tables on s3. diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index b51e402bff9..1e03f6f9fd8 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -867,7 +867,7 @@ bool FileCache::tryReserve( /// then we need to make sure that this fact that we fit in cache by size /// remains true after we release the lock and take it again. /// For this purpose we create a HoldSpace holder which makes sure that the space is hold in the meantime. - /// We substract reserve_stat.stat.releasable_size from the hold space, + /// We subtract reserve_stat.stat.releasable_size from the hold space, /// because it is the space that will be released, so we do not need to take it into account. const size_t hold_size = reached_size_limit ? size > reserve_stat.stat.releasable_size ? size - reserve_stat.stat.releasable_size : 0 diff --git a/src/Interpreters/Cache/IFileCachePriority.h b/src/Interpreters/Cache/IFileCachePriority.h index c5d440df915..f275dfa485a 100644 --- a/src/Interpreters/Cache/IFileCachePriority.h +++ b/src/Interpreters/Cache/IFileCachePriority.h @@ -41,6 +41,15 @@ public: bool isEvicting(const CachePriorityGuard::Lock &) const { return evicting; } bool isEvicting(const LockedKey &) const { return evicting; } + /// This does not look good to have isEvicting with two options for locks, + /// but still it is valid as we do setEvicting always under both of them. + /// (Well, not always - only always for setting it to True, + /// but for False we have lower guarantees and allow a logical race, + /// physical race is not possible because the value is atomic). + /// We can avoid this ambiguity for isEvicting by introducing + /// a separate lock `EntryGuard::Lock`, it will make this part of code more coherent, + /// but it will introduce one more mutex while it is avoidable. + /// Introducing one more mutex just for coherency does not win the trade-off (isn't it?). void setEvicting(bool evicting_, const LockedKey * locked_key, const CachePriorityGuard::Lock * lock) const { if (evicting_ && (!locked_key || !lock)) diff --git a/src/Interpreters/Cache/SLRUFileCachePriority.cpp b/src/Interpreters/Cache/SLRUFileCachePriority.cpp index 99edfe7fa33..07dd36e7655 100644 --- a/src/Interpreters/Cache/SLRUFileCachePriority.cpp +++ b/src/Interpreters/Cache/SLRUFileCachePriority.cpp @@ -10,11 +10,6 @@ namespace DB { -namespace ErrorCodes -{ - extern const int LOGICAL_ERROR; -} - namespace { size_t getRatio(size_t total, double ratio)