Cache HEAD API requests to object storage in the plain_rewritable disk #70915

jkartseva · 2024-10-22T09:26:10Z

Cache last modified metadata object in addition to file size.
Use metadata object cache to determine if a file exists.
Evict from cache when metadata is unlinked.

Changelog category (leave one):

Improvement

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

Reduce the number of object storage HEAD API requests in the plain_rewritable disk.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Exclude: Style check
Exclude: Fast test
Exclude: All with ASAN
Exclude: All with TSAN, MSAN, UBSAN, Coverage
Exclude: All with aarch64, release, debug

Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, etc.)
Exclude: AST fuzzers

robot-clickhouse-ci-1 · 2024-10-22T09:28:19Z

This is an automated comment for commit 91d7b7b with description of existing statuses. It's updated for the latest CI running

✅ Click here to open a full report in a separate page

Successful checks

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
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ 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. Integration 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
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
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	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
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

src/Disks/ObjectStorages/InMemoryPathMap.h

src/Disks/ObjectStorages/IMetadataStorage.h

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorage.cpp

src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.cpp

- Cache last modified metadata object in addition to file size. - Use metadata object cache to determine if a file exists. - Evict from cache when metadata is unlinked.

kssenii · 2024-10-24T09:54:54Z

src/Disks/ObjectStorages/MetadataStorageFromPlainRewritableObjectStorage.h

@@ -13,21 +13,28 @@ class MetadataStorageFromPlainRewritableObjectStorage final : public MetadataSto
 {
 private:
    const std::string metadata_key_prefix;
-    std::shared_ptr<InMemoryPathMap> path_map;
+    std::shared_ptr<InMemoryDirectoryPathMap> path_map;


I'd rename path_map as well, to something like directory_path_map, or directory_to_remote_info_map, or something else...

kssenii · 2024-10-24T10:02:22Z

src/Disks/ObjectStorages/MetadataStorageFromPlainObjectStorage.cpp

+        auto object_key = object_storage->generateObjectKeyForPath(path, std::nullopt /* key_prefix */);
+        if (auto metadata = object_storage->tryGetObjectMetadata(object_key.serialize()))
+            return std::make_shared<ObjectMetadataEntry>(metadata->size_bytes, metadata->last_modified.epochTime());
+        return nullptr;


If we once failed to get object metadata, for example, because s3 was temporarily unavailable, then looks like we will add nullptr to cache for this path and it would always be nullptr and never updated, should we instead check the error code because of which we failed to get metadata, and if it is key_doesnt_exist, then probably ok to leave nullptr in cache, but for another reason seems better not to cache it?

Fixed so null pointer is not cached.
I think storing a nullptr for non-existed key would be wrong in the present implementation, since I don't update the cache when a file is written.

Remote paths do not change.

…89e457b9669b7a9e9084987ca144b Cherry pick #70915 to 24.8: Cache HEAD API requests to object storage in the plain_rewritable disk

… the plain_rewritable disk

…89e457b9669b7a9e9084987ca144b Cherry pick #70915 to 24.9: Cache HEAD API requests to object storage in the plain_rewritable disk

… the plain_rewritable disk

Backport #70915 to 24.8: Cache HEAD API requests to object storage in the plain_rewritable disk

Backport #70915 to 24.9: Cache HEAD API requests to object storage in the plain_rewritable disk

jkartseva added the can be tested Allows running workflows for external contributors label Oct 22, 2024

jkartseva marked this pull request as draft October 22, 2024 09:26

robot-ch-test-poll2 added the pr-improvement Pull request with some product improvements label Oct 22, 2024

kssenii self-assigned this Oct 22, 2024

jkartseva force-pushed the reduce-api-calls-cost branch 2 times, most recently from c8f2f6e to c8685b1 Compare October 23, 2024 00:21

jkartseva changed the title ~~Cache HEAD API requests to object_storage in the plain_rewritable disk~~ Cache HEAD API requests to object storage in the plain_rewritable disk Oct 23, 2024

jkartseva force-pushed the reduce-api-calls-cost branch 5 times, most recently from 2c27c2e to a2859a1 Compare October 23, 2024 05:56

jkartseva marked this pull request as ready for review October 23, 2024 05:57

kssenii reviewed Oct 23, 2024

View reviewed changes

jkartseva added 3 commits October 24, 2024 00:05

Caching for HEAD API requests in the plain_rw disk

95ebbd9

- Cache last modified metadata object in addition to file size. - Use metadata object cache to determine if a file exists. - Evict from cache when metadata is unlinked.

rename a setting

8204279

add test

0b88b01

jkartseva force-pushed the reduce-api-calls-cost branch from a2859a1 to 2f533cb Compare October 24, 2024 03:46

jkartseva added 2 commits October 24, 2024 03:50

review fixes

7002863

rename InMemoryPathMap -> InMemoryDirectoryPathMap

2e38791

jkartseva force-pushed the reduce-api-calls-cost branch from 2f533cb to 2e38791 Compare October 24, 2024 03:50

kssenii reviewed Oct 24, 2024

View reviewed changes

kssenii approved these changes Oct 24, 2024

View reviewed changes

jkartseva added 2 commits October 24, 2024 18:00

review fix

acbf3d2

use remote path as the key cache instead of local

91d7b7b

Remote paths do not change.

jkartseva added this pull request to the merge queue Oct 24, 2024

Merged via the queue into ClickHouse:master with commit 2e61f2b Oct 25, 2024
211 checks passed

jkartseva deleted the reduce-api-calls-cost branch October 25, 2024 00:27

robot-ch-test-poll3 added the pr-synced-to-cloud The PR is synced to the cloud repo label Oct 25, 2024

jkartseva added the v24.10-must-backport label Oct 29, 2024

robot-ch-test-poll added the pr-backports-created-cloud label Oct 29, 2024

robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 29, 2024

robot-clickhouse added the pr-backports-created-cloud label Oct 30, 2024

jkartseva added a commit that referenced this pull request Oct 31, 2024

Merge pull request #71281 from ClickHouse/cherrypick/24.8/2e61f2bb991…

7480c3a

…89e457b9669b7a9e9084987ca144b Cherry pick #70915 to 24.8: Cache HEAD API requests to object storage in the plain_rewritable disk

robot-clickhouse added a commit that referenced this pull request Oct 31, 2024

Backport #70915 to 24.8: Cache HEAD API requests to object storage in…

9e97278

… the plain_rewritable disk

robot-clickhouse mentioned this pull request Oct 31, 2024

Backport #70915 to 24.8: Cache HEAD API requests to object storage in the plain_rewritable disk #71287

Merged

jkartseva added a commit that referenced this pull request Oct 31, 2024

Merge pull request #71282 from ClickHouse/cherrypick/24.9/2e61f2bb991…

8e89f30

…89e457b9669b7a9e9084987ca144b Cherry pick #70915 to 24.9: Cache HEAD API requests to object storage in the plain_rewritable disk

robot-clickhouse added a commit that referenced this pull request Oct 31, 2024

Backport #70915 to 24.9: Cache HEAD API requests to object storage in…

732b467

… the plain_rewritable disk

robot-ch-test-poll1 mentioned this pull request Oct 31, 2024

Backport #70915 to 24.9: Cache HEAD API requests to object storage in the plain_rewritable disk #71289

Merged

robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Oct 31, 2024

jkartseva pushed a commit that referenced this pull request Oct 31, 2024

Backport #70915 to 24.8: Cache HEAD API requests to object storage in…

fbce152

… the plain_rewritable disk

jkartseva pushed a commit that referenced this pull request Oct 31, 2024

Backport #70915 to 24.9: Cache HEAD API requests to object storage in…

675d524

… the plain_rewritable disk

jkartseva removed the pr-backports-created-cloud label Oct 31, 2024

robot-ch-test-poll3 added a commit that referenced this pull request Oct 31, 2024

Merge pull request #71287 from ClickHouse/backport/24.8/70915

25207bf

Backport #70915 to 24.8: Cache HEAD API requests to object storage in the plain_rewritable disk

robot-ch-test-poll4 added the pr-backports-created-cloud label Nov 1, 2024

robot-ch-test-poll added a commit that referenced this pull request Dec 4, 2024

Merge pull request #71289 from ClickHouse/backport/24.9/70915

163809e

Backport #70915 to 24.9: Cache HEAD API requests to object storage in the plain_rewritable disk

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache HEAD API requests to object storage in the plain_rewritable disk #70915

Cache HEAD API requests to object storage in the plain_rewritable disk #70915

jkartseva commented Oct 22, 2024 •

edited

Loading

robot-clickhouse-ci-1 commented Oct 22, 2024 •

edited

Loading

kssenii Oct 24, 2024

kssenii Oct 24, 2024

jkartseva Oct 24, 2024

Cache HEAD API requests to object storage in the plain_rewritable disk #70915

Cache HEAD API requests to object storage in the plain_rewritable disk #70915

Conversation

jkartseva commented Oct 22, 2024 • edited Loading

Changelog category (leave one):

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

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing):

robot-clickhouse-ci-1 commented Oct 22, 2024 • edited Loading

kssenii Oct 24, 2024

Choose a reason for hiding this comment

kssenii Oct 24, 2024

Choose a reason for hiding this comment

jkartseva Oct 24, 2024

Choose a reason for hiding this comment

jkartseva commented Oct 22, 2024 •

edited

Loading

robot-clickhouse-ci-1 commented Oct 22, 2024 •

edited

Loading