Make local object storage work consistently with s3 object storage, fix problem with append, make it configurable as independent storage #48791

kssenii · 2023-04-14T17:32:56Z

Changelog category (leave one):

Backward Incompatible Change

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Make local object storage work consistently with s3 object storage, fix problem with append (closes #48465), make it configurable as independent storage. The change is backward incompatible because cache on top of local object storage is not incompatible to previous versions.

…ct-storage

kssenii · 2023-04-21T10:54:12Z

tests/queries/0_stateless/02344_show_caches.reference

@@ -1,14 +0,0 @@
-cached_azure


removed this test because it is impossible to make it not flaky with custom nested (cached) disk configuration which I used in test tests/queries/0_stateless/02714_local_object_storage.sql

kssenii · 2023-04-21T10:55:59Z

src/Disks/ObjectStorages/Cached/CachedObjectStorage.cpp

-            CurrentThread::isInitialized() && CurrentThread::get().getQueryContext() ? std::string(CurrentThread::getQueryId()) : "",
-            file_size.value(),
-            /* allow_seeks */true,
-            /* use_external_buffer */false);


Removed this code because it is not needed when local object storage works the same way as s3 object storage (and in this PR is starts to), as cache is added on the different layer.

kssenii · 2023-04-21T10:56:36Z

src/Disks/DiskLocal.cpp

@@ -54,53 +51,6 @@ std::mutex DiskLocal::reservation_mutex;

 using DiskLocalPtr = std::shared_ptr<DiskLocal>;

-static void loadDiskLocalConfig(const String & name,


this code is just moved to a separate file, see src/Disks/loadLocalDiskConfig.cpp

…ct-storage

…o better-local-object-storage

…ct-storage

…o better-local-object-storage

robot-clickhouse-ci-1 · 2023-05-01T20:26:14Z

This is an automated comment for commit 35f437a with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🔴 failure

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	🟢 success
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	🟢 success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	🟢 success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	🟢 success
Docker image for servers	The check to build and optionally push the mentioned image to docker hub	🟢 success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	🟢 success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integrational tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	🟢 success
Install packages	Checks that the built packages are installable in a clear environment	🟢 success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	🟢 success
Mergeable Check	Checks if all other necessary checks are successful	🟢 success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	🟢 success
Push to Dockerhub	The check for building and pushing the CI related docker images to docker hub	🟢 success
SQLancer	Fuzzing tests that detect logical bugs with SQLancer tool	🟢 success
Sqllogic	Run clickhouse on the sqllogic test set against sqlite and checks that all statements are passed	🟢 success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🟢 success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	🔴 failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	🔴 failure
Style Check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	🟢 success
Unit tests	Runs the unit tests for different release types	🟢 success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	🟢 success

alesapin · 2023-05-03T10:08:02Z

src/Disks/IDisk.h

@@ -367,7 +367,13 @@ class IDisk : public Space
    /// Actually it's a part of IDiskRemote implementation but we have so
    /// complex hierarchy of disks (with decorators), so we cannot even
    /// dynamic_cast some pointer to IDisk to pointer to IDiskRemote.
-    virtual MetadataStoragePtr getMetadataStorage() = 0;
+    virtual MetadataStoragePtr getMetadataStorage()


Why we have implementation now?

because no need to require implementation of this method from all disks, only from DiskObjectStorage, so it is ok to throw NOT_IMPLEMENTED here.

alesapin · 2023-05-03T10:09:59Z

src/Disks/ObjectStorages/Cached/CachedObjectStorage.cpp

@@ -59,7 +59,7 @@ ReadSettings CachedObjectStorage::patchSettings(const ReadSettings & read_settin
    if (!canUseReadThroughCache())
        modified_settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache = true;

-    return IObjectStorage::patchSettings(modified_settings);
+    return object_storage->patchSettings(modified_settings);


So it was a bug before this change?

Not really, it did not affect anything, when readObjects is passed to inner object storage (s3) it will apply it's own patch settings for its read.

alesapin · 2023-05-03T10:11:37Z

src/Disks/ObjectStorages/StoredObject.h

@@ -18,31 +18,14 @@ struct StoredObject

    uint64_t bytes_size = 0;

-    std::string getPathKeyForCache() const;
-
    const std::string & getMappedPath() const;


Why we have getter for mapped path and don't have getter for absolute path? Maybe it should be const?

Why we have getter for mapped path and don't have getter for absolute path?

fixed

Maybe it should be const

copy assignment operator does not work then

alesapin · 2023-05-03T10:14:28Z

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp

-    fs::copy(from, to, fs::copy_options::recursive | fs::copy_options::overwrite_existing);
+    auto in = readObject(object_from);
+    auto out = writeObject(object_to, WriteMode::Rewrite);
+    copyData(*in, *out);


Why we need to read/write? Why we cannot copy blob from object_from to object_to?

because the point is to not use std::filesystem::copy (for #42337)

alesapin · 2023-05-03T10:14:47Z

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp

-    LOG_TEST(log, "Write object: {}", path);
-    return std::make_unique<WriteBufferFromFile>(path, buf_size, flags);
+    if (mode != WriteMode::Rewrite)
+        throw Exception(ErrorCodes::BAD_ARGUMENTS, "LocalObjectStorage doesn't support append to files");


It can be really surprizing

same as in S3ObjectStorage, they are consistent now.

alesapin · 2023-05-03T10:28:22Z

src/Disks/ObjectStorages/Local/LocalObjectStorage.cpp

@@ -47,30 +50,39 @@ std::unique_ptr<ReadBufferFromFileBase> LocalObjectStorage::readObjects( /// NOL
    std::optional<size_t> read_hint,
    std::optional<size_t> file_size) const
 {
-    if (objects.size() != 1)


We should still have this check since we banned append in write?

No, append is not banned, it is now implemented same as in s3, which means that here objects.size() != 1 in case append was made.

alesapin · 2023-05-03T10:31:42Z

src/Disks/IDisk.cpp

@@ -7,9 +7,6 @@
 #include <Common/logger_useful.h>
 #include <Common/setThreadName.h>
 #include <Core/ServerUUID.h>
-#include <Disks/ObjectStorages/MetadataStorageFromDisk.h>
-#include <Disks/ObjectStorages/FakeMetadataStorageFromDisk.h>


Maybe we can remove whole FakeMetadataStorageFromDisk now?

alesapin · 2023-05-03T10:32:00Z

src/Core/Settings.h

@@ -660,7 +660,6 @@ class IColumn;
    M(Bool, enable_filesystem_cache_on_write_operations, false, "Write into cache on write operations. To actually work this setting requires be added to disk config too", 0) \
    M(Bool, enable_filesystem_cache_log, false, "Allows to record the filesystem caching log for each query", 0) \
    M(Bool, read_from_filesystem_cache_if_exists_otherwise_bypass_cache, false, "Allow to use the filesystem cache in passive mode - benefit from the existing cache entries, but don't put more entries into the cache. If you set this setting for heavy ad-hoc queries and leave it disabled for short real-time queries, this will allows to avoid cache threshing by too heavy queries and to improve the overall system efficiency.", 0) \
-    M(Bool, enable_filesystem_cache_on_lower_level, true, "If read buffer supports caching inside threadpool, allow it to do it, otherwise cache outside ot threadpool. Do not use this setting, it is needed for testing", 0) \


alesapin · 2023-05-03T10:35:06Z

src/Disks/DiskType.h

@@ -35,6 +36,8 @@ inline String toString(DataSourceType data_source_type)
            return "web";
        case DataSourceType::AzureBlobStorage:
            return "azure_blob_storage";
+        case DataSourceType::LocalBlobStorage:


Looks like now it's impossible to create cache on top of <type>local</type>. Not a popular feature, but need to mention in changelog.

alesapin · 2023-05-03T10:36:55Z

src/Disks/ObjectStorages/Local/LocalObjectStorage.h

-    std::string generateBlobNameForPath(const std::string & path) override { return path; }
-
-    std::string getUniqueId(const std::string & path) const override;
+    std::string generateBlobNameForPath(const std::string & path) override;


Maybe this function can be 100% the same for all object storages?

For local no, because it will require to create subdirectories.

alesapin · 2023-05-04T09:47:37Z

Need to address stress test failure, rest is LGTM

Better local object storage

c49ff08

robot-ch-test-poll4 added the pr-improvement Pull request with some product improvements label Apr 14, 2023

UnamedRus mentioned this pull request Apr 14, 2023

RFC: disk without rename/hardlink/mv/copy operations #42337

Closed

Deprecetae local object storage on fake metadata storage

9374666

kssenii force-pushed the better-local-object-storage branch from 9934483 to 9374666 Compare April 15, 2023 11:53

kssenii marked this pull request as draft April 16, 2023 19:07

kssenii added 9 commits April 17, 2023 16:41

Better

d2c73a5

Merge remote-tracking branch 'upstream/master' into better-local-obje…

b77e9c1

…ct-storage

Commit forgotten file

86ca6c4

Fix style check

a470dea

Merge remote-tracking branch 'upstream/master' into better-local-obje…

16b027e

…ct-storage

Fix

0669306

Fix tests

277c616

Remove redundant code

fef179a

Fix tests

0b2f693

kssenii marked this pull request as ready for review April 20, 2023 10:34

kssenii added 3 commits April 21, 2023 12:44

Fix upgrade check, fix integration test

88f412a

Merge remote-tracking branch 'upstream/master' into better-local-obje…

e58c163

…ct-storage

Minor change

071e9a6

kssenii commented Apr 21, 2023

View reviewed changes

devcrafter self-assigned this Apr 21, 2023

KochetovNicolai approved these changes Apr 21, 2023

View reviewed changes

kssenii and others added 7 commits April 21, 2023 22:10

Update run.sh

370aa60

Update 02714_local_object_storage.sql

93563ad

Merge remote-tracking branch 'upstream/master' into better-local-obje…

d802380

…ct-storage

Merge remote-tracking branch 'origin/better-local-object-storage' int…

24b7532

…o better-local-object-storage

Update run.sh

f38dfc9

Update run.sh

c4abe66

Merge branch 'master' into better-local-object-storage

45e2d29

kssenii requested a review from devcrafter April 28, 2023 10:26

kssenii and others added 5 commits April 28, 2023 13:26

Merge remote-tracking branch 'upstream/master' into better-local-obje…

298ed45

…ct-storage

Merge remote-tracking branch 'upstream/master' into better-local-obje…

e603adf

…ct-storage

Merge branch 'master' into better-local-object-storage

d4aa96e

Better

f325ba1

Merge remote-tracking branch 'origin/better-local-object-storage' int…

fbdccbd

…o better-local-object-storage

Simplify

4fc1f64

kssenii requested review from devcrafter and removed request for devcrafter May 2, 2023 12:29

alesapin reviewed May 3, 2023

View reviewed changes

Address review comments

35f437a

kssenii force-pushed the better-local-object-storage branch from 343d44f to 35f437a Compare May 3, 2023 12:37

alesapin approved these changes May 4, 2023

View reviewed changes

alesapin merged commit 412b161 into ClickHouse:master May 4, 2023
259 of 261 checks passed

kssenii assigned alesapin May 4, 2023

tavplubix mentioned this pull request May 9, 2023

Hung check failed because caches are too slow #49714

Open

baibaichen mentioned this pull request May 22, 2023

[GLUTEN-1632][CH]Update Clickhouse Version (20230517) apache/incubator-gluten#1692

Merged

UnamedRus mentioned this pull request Jun 30, 2023

NAS zero copy replication #30730

Closed

UnamedRus mentioned this pull request Jul 13, 2023

In local storage, Is it possible to save the *.bin files into hdd when other files into ssd? #52093

Closed

UnamedRus mentioned this pull request Aug 22, 2023

How to use cache disk over a local disk? #53670

Closed

This was referenced Aug 22, 2023

Doc. Cache does not support local disks anymore den-crane/ClickHouse#1

Closed

Doc. Cache is not supported for local disks anymore #53707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make local object storage work consistently with s3 object storage, fix problem with append, make it configurable as independent storage #48791

Make local object storage work consistently with s3 object storage, fix problem with append, make it configurable as independent storage #48791

kssenii commented Apr 14, 2023 •

edited

kssenii Apr 21, 2023 •

edited

kssenii Apr 21, 2023

kssenii Apr 21, 2023

robot-clickhouse-ci-1 commented May 1, 2023 •

edited by robot-ch-test-poll

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023

alesapin May 3, 2023

alesapin May 3, 2023

alesapin May 3, 2023

kssenii May 3, 2023 •

edited

alesapin commented May 4, 2023

		@@ -54,53 +51,6 @@ std::mutex DiskLocal::reservation_mutex;

		using DiskLocalPtr = std::shared_ptr<DiskLocal>;

		static void loadDiskLocalConfig(const String & name,

Make local object storage work consistently with s3 object storage, fix problem with append, make it configurable as independent storage #48791

Make local object storage work consistently with s3 object storage, fix problem with append, make it configurable as independent storage #48791

Conversation

kssenii commented Apr 14, 2023 • edited

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

kssenii Apr 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robot-clickhouse-ci-1 commented May 1, 2023 • edited by robot-ch-test-poll

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kssenii May 3, 2023 • edited

Choose a reason for hiding this comment

alesapin commented May 4, 2023

kssenii commented Apr 14, 2023 •

edited

kssenii Apr 21, 2023 •

edited

robot-clickhouse-ci-1 commented May 1, 2023 •

edited by robot-ch-test-poll

kssenii May 3, 2023 •

edited