Base implementation of IDisk interafce for S3#8649
Base implementation of IDisk interafce for S3#8649alexey-milovidov merged 5 commits intoClickHouse:masterfrom
Conversation
There was a problem hiding this comment.
What's a magic number 17?
There was a problem hiding this comment.
Current s3 object name is root_path + path + '.' + random string of 16 chars. Here I cut random suffix with a dot.
dbms/src/Disks/DiskS3.h
Outdated
There was a problem hiding this comment.
The clearDirectory method should removes only files (see #8356 (comment)). Probably we need to add some option to clearDirectory for removing directories as well.
dbms/src/Storages/StorageTinyLog.cpp
Outdated
There was a problem hiding this comment.
Shouldn't table_path itself be deleted as well?
There was a problem hiding this comment.
Then we need to add void remove(const String & path, bool recursive); in IDisk interface.
dbms/src/Disks/DiskS3.cpp
Outdated
There was a problem hiding this comment.
To better decouple data and metadata I'd generate s3_to_path something like s3_root_path + '.storage/' + getRandomFilename().
There was a problem hiding this comment.
I had preferred to add origin filename to make bucket content more human-readable and reduce the probability of name overlap.
GrigoryPervakov
force-pushed
the
vfs-s3
branch
from
10925f6 to
36e5322
Compare
|
|
||
| auto file_it = files.find(path); | ||
| if (file_it == files.end()) | ||
| return; |
There was a problem hiding this comment.
Why don't throw an exception?
| /** | ||
| * Storage for persisting data in S3 and metadata on the local disk. | ||
| * Files are represented by file in local filesystem (clickhouse_root/disks/disk_name/path/to/file) | ||
| * that contains S3 object key with actual data. |
There was a problem hiding this comment.
And also should contain file size.
|
|
||
| public: | ||
| /// Used for reservation counters modification | ||
| static std::mutex reservationMutex; |
| void finalize() override | ||
| { | ||
| WriteBufferFromS3::finalize(); | ||
| Poco::FileOutputStream(metadata_path) << s3_path; |
There was a problem hiding this comment.
Not sure if error is checked.
3 participants
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):
Add base implementation for IDisk interface for S3 storage