Removed forceful drop cache command, fix detached status state by kssenii · Pull Request #36825 · ClickHouse/ClickHouse

kssenii · 2022-04-30T13:03:34Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

tavplubix · 2022-05-02T17:53:26Z

src/Common/FileCache.cpp

+        throw Exception(
+            ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR,
+            "Cache cell already exists for key `{}` and offset {}",
+            keyToStr(key), offset);


Looks like LOGICAL_ERROR

src/Common/FileSegment.cpp

alesapin

Need clarification.

alesapin · 2022-05-08T16:00:41Z

src/Common/FileSegment.cpp

        throw Exception(ErrorCodes::REMOTE_FS_OBJECT_CACHE_ERROR, "Zero space reservation is not allowed");

-    assertNotDetached();
+    std::lock_guard cache_lock(cache->mutex);


Comment, why we need both locks here.

alesapin · 2022-05-08T16:01:04Z

src/Common/FileSegment.cpp


-    assertNotDetached();
+    std::lock_guard cache_lock(cache->mutex);
+    std::lock_guard detach_lock(detach_mutex);


Very complex, taking three mutexes at once. Need detailed explanation.

src/Common/FileSegment.cpp

alesapin · 2022-05-08T16:02:39Z

src/Common/FileSegment.cpp

 }

-void FileSegment::assertNotDetached() const
+void FileSegment::throwDetached() const


throwIfDetached?

alesapin · 2022-05-08T16:04:08Z

src/Common/FileSegment.h

    static FileSegmentPtr getSnapshot(const FileSegmentPtr & file_segment, std::lock_guard<std::mutex> & cache_lock);

-    void detach(std::lock_guard<std::mutex> & cache_lock, std::lock_guard<std::mutex> & segment_lock);
+        void detach(


Suggested change

void detach(

void detach(

src/Common/FileSegment.cpp

alesapin · 2022-05-08T16:20:02Z

src/Common/FileSegment.cpp

                        size, offset_, download_offset);

-    assertNotDetached();
+    std::lock_guard detach_lock(detach_mutex);


Why we need it here? Add a comment.

alesapin · 2022-05-08T16:20:44Z

src/Common/FileSegment.cpp

    std::lock_guard segment_lock(mutex);
+
+    if (is_forcefully_detached)
+        throwDetachedUnlocked(segment_lock);


it's unexpected that method with is prefix can throw an exception.

alesapin · 2022-05-08T16:22:06Z

src/Common/FileSegment.cpp

        /// needed, downloader is set on file segment creation).
        case (State::DOWNLOADING):
        {
+            /// On write-through cache we do not check downloader id.


What does it mean? Where we don't check it?

alesapin · 2022-05-08T16:24:17Z

src/IO/WriteBufferFromS3.cpp

    {
        UploadPartTask task;
        fillUploadRequest(task.req, part_tags.size() + 1);
-        if (file_segments_holder)


Unclear changes to me.

kssenii · 2022-05-10T17:25:37Z

will delete the forceful drop cache command instead

kssenii · 2022-05-11T08:28:00Z

Some changes still make sense, will remove only what is not needed with removal of forced detach

kssenii · 2022-05-11T08:55:22Z

src/Common/FileSegment.cpp

+    download_state = State::PARTIALLY_DOWNLOADED_NO_CONTINUATION;
+    downloader_id.clear();


the most important change here, though its absence should not affect anything because now detached status can be in 2 cases, which do not do any complex logic and check exactly for detached status:

there is only 1 remaining file segment holder && it does not need this segment anymore && this file segment was in cache and needs to be removed

in read_from_cache_if_exists_otherwise_bypass_cache case (get method can create them)

So this change not required, but it is more correct to have it (so the file segment state becomes consistent). Other remaining changes in this pr are just assertions and adding logical errors in some places.

alesapin

Small comments.

alesapin · 2022-05-11T21:06:06Z

src/Common/FileSegment.h

    /// In general case, all file segments are owned by cache.
-    bool detached = false;
+    bool is_detached = false;
+    bool is_forcefully_detached = false;


alesapin · 2022-05-11T21:08:35Z

src/Common/FileSegment.cpp


    markAsDetached(segment_lock);
+    download_state = State::PARTIALLY_DOWNLOADED_NO_CONTINUATION;
+    downloader_id.clear();


Add this comment to the code

alesapin · 2022-05-11T21:09:15Z

src/Common/FileSegment.cpp

+        throw Exception(
+            ErrorCodes::LOGICAL_ERROR,
+            "Cache file segment is in detached state, operation not allowed. "
+            "It can happen when cache was concurrently dropped with SYSTEM DROP FILESYSTEM CACHE FORCE");


Is it still required?

alesapin · 2022-05-12T15:18:19Z

Failure unrelated.

Debug

eea0a00

robot-clickhouse added the pr-not-for-changelog This PR should not be mentioned in the changelog label Apr 30, 2022

kssenii mentioned this pull request Apr 30, 2022

Minor Coverity defects fixes #36632

Merged

Fix

8cf83a2

azat mentioned this pull request May 2, 2022

[RFC] Remove unimplemented RAID1 support #36836

Merged

kssenii changed the title ~~Fix hung check~~ Reimplement write-through cache May 2, 2022

kssenii assigned alesapin May 2, 2022

kssenii force-pushed the cache-fix-1 branch 2 times, most recently from 761147d to 2491514 Compare May 2, 2022 16:53

kssenii marked this pull request as ready for review May 2, 2022 16:54

Reimplement write-through cache

70e2bb6

kssenii force-pushed the cache-fix-1 branch from 2491514 to 70e2bb6 Compare May 2, 2022 16:55

tavplubix reviewed May 2, 2022

View reviewed changes

alesapin and others added 2 commits May 3, 2022 00:28

Better fix for cache

73f3334

Tiny changes

a3c9ff7

kssenii force-pushed the cache-fix-1 branch from 8010f57 to a3c9ff7 Compare May 3, 2022 09:46

kssenii changed the title ~~Reimplement write-through cache~~ Fix stress test May 3, 2022

kssenii force-pushed the cache-fix-1 branch from ae4a5c9 to 2f4df5c Compare May 3, 2022 11:20

Better

b0d43e3

kssenii force-pushed the cache-fix-1 branch from 2f4df5c to b0d43e3 Compare May 3, 2022 12:20

Merge master

025e632

azat mentioned this pull request May 3, 2022

Implement least_used load balancing algorithm for disks inside volume #36686

Merged

tavplubix mentioned this pull request May 4, 2022

Add extra info when sending exception #36872

Merged

This was referenced May 4, 2022

Add column to system.filesystem_cache_log #36874

Merged

quick tmp fix for stress test #36900

Merged

Fix lock inversion

c8b5dec

kssenii force-pushed the cache-fix-1 branch from 25ca6ff to c8b5dec Compare May 4, 2022 12:29

kssenii closed this in #36900 May 4, 2022

kssenii reopened this May 4, 2022

kssenii force-pushed the cache-fix-1 branch from 6bcdf70 to 48c2053 Compare May 5, 2022 13:40

Merge master

78d3fd5

kssenii force-pushed the cache-fix-1 branch from 48c2053 to 78d3fd5 Compare May 5, 2022 15:41

kssenii added 8 commits May 6, 2022 11:21

Merge master

2275196

Do not throw logical error for detached file segment

23d9230

Merge master

35ed60f

Fix build

7e99135

Merge master

0db83d7

Fix assert in case cache forcefully dropped

0e9e1dd

Fix exception message

50d2296

Fix incorrect assetion

f43c09b

alesapin requested changes May 8, 2022

View reviewed changes

kssenii closed this May 10, 2022

kssenii added 2 commits May 10, 2022 19:33

Merge master

ccab49e

Remove unneede changes

18dd74b

kssenii reopened this May 11, 2022

Fix review comments

5385a4d

kssenii changed the title ~~Fix stress test~~ Removed forceful drop cache command, fix detached status state May 11, 2022

kssenii commented May 11, 2022

View reviewed changes

kssenii added 3 commits May 11, 2022 12:05

Update FileSegment.cpp

a440e13

Update test

28630b2

Update 02286_drop_filesystem_cache.reference

567c9e6

alesapin approved these changes May 11, 2022

View reviewed changes

kssenii added 3 commits May 11, 2022 23:33

Update FileSegment.cpp

2d15520

Update FileSegment.h

4483e57

Update FileSegment.cpp

d636a02

alesapin approved these changes May 12, 2022

View reviewed changes

alesapin merged commit e7296a2 into ClickHouse:master May 12, 2022

		download_state = State::PARTIALLY_DOWNLOADED_NO_CONTINUATION;
		downloader_id.clear();

Conversation

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

Uh oh!

Changelog category (leave one):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kssenii commented May 10, 2022

Uh oh!

kssenii commented May 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alesapin commented May 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kssenii commented Apr 30, 2022 •

edited

Loading