Local cache for remote filesystem by kssenii · Pull Request #33717 · ClickHouse/ClickHouse

kssenii · 2022-01-17T23:45:20Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add local cache for disk s3. Closes #28961.

Detailed description / Documentation draft:
.

yiguolei · 2022-01-18T06:14:03Z

If the read request is from merge operation, then it will read a lot of data and write to local cache but they will be deleted after merge finished. Is there a better way to avoid this?

kssenii · 2022-01-18T10:32:51Z

@yiguolei

If the read request is from merge operation, then it will read a lot of data and write to local cache but they will be deleted after merge finished. Is there a better way to avoid this?

Thanks, you are right, will make a solution to avoid this.

xiedeyantu · 2022-01-21T14:27:57Z

I compiled using this branch, but I made an error at startup.
Application: DB::Exception: Cannot read all data. Bytes read: 3. Bytes expected: 4.

kssenii · 2022-01-21T14:29:42Z

It is not ready yet. Moreover I did not push the latest version in this PR.
And it is in draft state now, you can test it when it is not a draft.

kant777 · 2022-01-24T06:16:35Z

https://dbs.uni-leipzig.de/file/ARC.pdf

alexey-milovidov · 2022-01-24T07:08:46Z

@kant777 LRU is obviously not the best caching algorithm, but it is pointless to implement anything else until you have reproducible comparison mechanism to evaluate different algorithms on realistic workload.

alesapin

Still reading, but currently I see no big issues. Discussed a lot in TG chat. Let's continue.

src/Disks/IO/CacheableReadBufferFromRemoteFS.h

src/Common/FileCache.h

src/Common/FileCacheFactory.cpp

src/Disks/IO/ReadBufferFromRemoteFSGather.cpp

src/Common/FileCache.cpp

alesapin · 2022-01-24T12:06:43Z

src/Common/FileCache.cpp

+
+bool LRUFileCache::tryReserve(const Key & key_, size_t offset_, size_t size, [[maybe_unused]] std::lock_guard<std::mutex> & cache_lock)
+{
+    auto removed_size = 0;


Maybe check corner case when we are trying to reserve more data than the whole cache size?

src/Disks/IO/CacheableReadBufferFromRemoteFS.cpp

src/Common/FileSegment.cpp

kssenii · 2022-03-11T06:22:47Z

Stress test (thread, actions) — Sanitizer assert (in stderr.log)

Not related to changes:

WARNING: ThreadSanitizer: heap-use-after-free (pid=607)
  Atomic write of size 8 at 0x7b48006c01f0 by thread T243:
    #0 __tsan_atomic64_fetch_add <null> (clickhouse+0xad0a815)
    #1 long std::__1::__cxx_atomic_fetch_add<long>(std::__1::__cxx_atomic_base_impl<long>*, long, std::__1::memory_order) obj-x86_64-linux-gnu/../contrib/libcxx/include/atomic:1050:12 (clickhouse+0xad9d56a)
    #2 std::__1::__atomic_base<long, true>::fetch_add(long, std::__1::memory_order) obj-x86_64-linux-gnu/../contrib/libcxx/include/atomic:1719:17 (clickhouse+0xad9d56a)
    #3 MemoryTracker::allocImpl(long, bool, MemoryTracker*) obj-x86_64-linux-gnu/../src/Common/MemoryTracker.cpp:118:35 (clickhouse+0xad9d56a)
    #4 MemoryTracker::allocImpl(long, bool, MemoryTracker*) obj-x86_64-linux-gnu/../src/Common/MemoryTracker.cpp:239:22 (clickhouse+0xad9d940)

Stateless tests flaky check (address, actions) — Timeout, fail: 27, passed: 390

Those which failed - failed because of a timeout in stateless tests flakey check run.

alexey-milovidov · 2022-03-17T01:46:07Z

VFS cache is intended to be composable. So, you define caching layer on top of another VFS. So you can have client-side encrypted data on s3 and choose between caching encrypted and unencrypted data. Now I see that s3 cache configuration is duplicating s3 configuration 😿

I will have to say that configuration is preliminary and the feature is in preview.

azat · 2022-04-21T15:11:06Z

src/Common/FileSegment.cpp

+        if (allow_non_strict_checking)
+            return "None";
+
+        throw Exception(ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR, "Cannot use cache without query id");


Shouldn't be this a LOGICAL_ERROR?

Yeah, thanks, I will change in those places.
Initially I started making it REMOTE_FS_OBJECT_CACHE_ERROR instead of LOGICAL_ERROR, because I wanted to check certain errors for EXPECT_THROW in unit tests, but then it would not work in debug build unit tests, so I started to not use logical error...

azat · 2022-04-21T15:11:37Z

src/Common/FileSegment.cpp

+    std::lock_guard segment_lock(mutex);
+
+    if (downloader_id.empty())
+        throw Exception(ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR, "There is no downloader");


Same here, why not LOGICAL_ERROR?

azat · 2022-04-21T15:11:52Z

src/Common/FileSegment.cpp

+        throw Exception(ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR, "There is no downloader");
+
+    if (getCallerId() != downloader_id)
+        throw Exception(ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR, "Downloader can be reset only by downloader");


Also here, and in few other places in this file for similar exceptions.

azat · 2022-04-21T15:25:24Z

src/Disks/RemoteDisksCommon.cpp

 namespace ErrorCodes
-{
-    extern const int BAD_ARGUMENTS;
+{extern const int BAD_ARGUMENTS;


robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Jan 17, 2022

kssenii assigned alesapin Jan 17, 2022

kssenii force-pushed the local-cache-for-remote-fs branch from 7eaf6a5 to 41f4f33 Compare January 18, 2022 09:45

kssenii force-pushed the local-cache-for-remote-fs branch from 41f4f33 to 31d2749 Compare January 18, 2022 11:51

Local cache for remote filesystem

a566099

kssenii force-pushed the local-cache-for-remote-fs branch from 31d2749 to a566099 Compare January 18, 2022 21:42

kssenii added 3 commits January 21, 2022 18:39

Make space reservation incremental

1858f69

Fix threadpool reads

9f295eb

Better

36a41ac

kssenii force-pushed the local-cache-for-remote-fs branch from b5e12e1 to 36a41ac Compare January 22, 2022 17:43

kssenii and others added 4 commits January 22, 2022 22:53

Merge branch 'master' into local-cache-for-remote-fs

2c43e4e

Better

690db14

Better

cf49f41

Fix partially downloaded no continuation case

710bba8

kssenii force-pushed the local-cache-for-remote-fs branch from fa0780a to 710bba8 Compare January 23, 2022 20:49

This was referenced Jan 24, 2022

Roadmap 2022 (discussion) #32513

Closed

Add local cache for S3 objects #29638

Closed

Fix tests

898139a

alesapin reviewed Jan 24, 2022

View reviewed changes

kssenii force-pushed the local-cache-for-remote-fs branch 2 times, most recently from 1589782 to 8a7b4fa Compare January 24, 2022 22:34

Fixes

aef99de

kssenii force-pushed the local-cache-for-remote-fs branch from 8a7b4fa to aef99de Compare January 24, 2022 22:46

kssenii added 12 commits March 7, 2022 17:05

Fix detaching query

eeb0ae9

Fix split build

0737aef

Fix

4f53080

Fix tests

c20c745

Fix uncaught exception from destructor

28a3aec

Remove already fixed crutch

f3cba6f

Fix split build

e231c3a

Merge master

5260822

Fix build, update test

16e84c2

Update tests

8941614

Fix test, disable cache for DiskCacheWrapper

96789fa

Clean up code

6571c7f

kssenii force-pushed the local-cache-for-remote-fs branch from 569dfe8 to 20eb192 Compare March 9, 2022 12:38

Update test

aab3de7

kssenii force-pushed the local-cache-for-remote-fs branch from c717854 to aab3de7 Compare March 9, 2022 15:13

kssenii added 3 commits March 9, 2022 18:14

Fix race

d089b0a

Disable cache for memory test

6e53ee0

Update test

bd68d1d

kssenii force-pushed the local-cache-for-remote-fs branch from e7f7298 to bd68d1d Compare March 10, 2022 10:02

kssenii added 4 commits March 10, 2022 11:42

Merge master

787a080

Release test

d54b077

Fix test

ddcb020

Fix typo

c86955d

kssenii force-pushed the local-cache-for-remote-fs branch from c70f7f5 to c86955d Compare March 10, 2022 19:22

alesapin approved these changes Mar 10, 2022

View reviewed changes

kssenii merged commit 818459b into ClickHouse:master Mar 11, 2022

tavplubix mentioned this pull request Mar 29, 2022

Try to run stateful tests in parallel #35720

Merged

azat reviewed Apr 21, 2022

View reviewed changes

Conversation

kssenii commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yiguolei commented Jan 18, 2022

Uh oh!

kssenii commented Jan 18, 2022

Uh oh!

xiedeyantu commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kssenii commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kant777 commented Jan 24, 2022

Uh oh!

alexey-milovidov commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alesapin Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kssenii commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Mar 17, 2022

Uh oh!

azat Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

kssenii Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

azat Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

azat Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

azat Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kssenii commented Jan 17, 2022 •

edited

Loading

xiedeyantu commented Jan 21, 2022 •

edited

Loading

kssenii commented Jan 21, 2022 •

edited

Loading

alexey-milovidov commented Jan 24, 2022 •

edited

Loading

kssenii commented Mar 11, 2022 •

edited

Loading