New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
S3Express support #59965
S3Express support #59965
Conversation
This is an automated comment for commit f9d1c57 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
|
4154344
to
835b475
Compare
645bf8b
to
974ba73
Compare
|
What do you mean? |
|
||
namespace DB::S3 | ||
{ | ||
|
||
namespace Model = Aws::S3::Model; | ||
|
||
/// Used only for S3Express | ||
namespace RequestChecksum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This namespace seems to be redundant and it seems it makes the code less clear.
These functions are very simple and it seems you can just do what you want directly without these functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions are very simple and it seems you can just do what you want directly without these functions.
yep, let me explain what problem I tried to solve.
in Client::doRequest()
we do
if (client_settings.is_s3express_bucket)
request.setIsS3ExpressBucket();
that basically sets checksum algorithm. and then (much higher on the stack) in WriteBufferFromS3
we will have to set the appropriate checksum. and because there are multiple checksum algorithms (and consequently multiple SetChecksum*
methods) it looked really random to just call some crc
-related methods out of nowhere. so I thought that since we're gonna use only one algorithm inside CH codebase anyway - it would be a good idea to abstract it away inside some class or just a namespace.
if you still think it doesn't worth it - i'm ok to get rid of it.
{ | ||
if constexpr (requires { request.SetChecksumAlgorithm(Model::ChecksumAlgorithm::CRC32); }) | ||
request.SetChecksumAlgorithm(Model::ChecksumAlgorithm::CRC32); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This template function looks like a bit of overcomplication. Which actual types of requests need this function SetChecksumAlgorithm(Model::ChecksumAlgorithm::CRC32)
to be called? Only UploadPartRequest
? Why can't you just call it immediately after calling req.SetChecksumCRC32()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to my understanding it is needed for all requests with body. e.g. also CreateMultipartUploadRequest
, PutObjectRequest
.
BTW what is S3Express? And why it requires checksum to be sent? |
I mean the linked pr to docs: ClickHouse/clickhouse-docs#2069 |
https://aws.amazon.com/s3/storage-classes/express-one-zone/ |
Changelog category:
Changelog entry:
Implemented support for S3Express buckets.
Documentation entry for user-facing changes