Implement and test S3 Sink, with usage in runtime. #278
Conversation
There was a problem hiding this comment.
I don’t have clear blockers, but I think a round of cleanup and clarifications will be good before merging. There are several concerns that I have in general:
- I'm worried that the error handling patterns in this feature are unclear. Using catch-all for exceptions and the practice of throwing for catching and logging is especially concerning. Error handling for URIs also feels incomplete.
- I think we're using
noexceptinappropriately (primarily to convey intent), which could cause issues over time. - I might be missing some context, but the use of certain containers (
std::listandstd::queue) doesn’t make a lot of sense to me. These are not trivial decisions because they typically convey intent and come with overhead.
Anyway, I think it’s almost there.
src/writers/sink.creator.cpp
Outdated
| const auto object_key = object_keys.front(); | ||
| object_keys.pop(); |
There was a problem hiding this comment.
- Why do we need to pop all the objects? If it's not absolutely necessary we should use a vector.
- What happens if an object fails (an iteration throws an exception)? If we pop it from the queue, it's lost no?
There was a problem hiding this comment.
- I use the queue to construct paths, as we discussed in a previous PR. I'd have to rework that whole thing. I really don't think that a vector is the right tool for constructing paths like
/(1...k)/(1...m)/(1...n)/.../(1...z). - I've been thinking that since we're not actually doing anything resource intensive, like opening a file, when we create an S3Sink, we can do this serially.
There was a problem hiding this comment.
I think I’m missing something here. I’ll tinker with the code to better understand this requirement.
src/writers/s3.sink.hh
Outdated
| std::shared_ptr<common::S3ConnectionPool> connection_pool_; | ||
|
|
||
| // multipart upload | ||
| std::array<uint8_t, 5 << 20> part_buffer_; |
There was a problem hiding this comment.
I would restore the comment or create a variable (static constexpr size_t five_mb_size = 5 << 20) so it's obvious what this number represents.
src/writers/s3.sink.cpp
Outdated
| auto get_upload_id = [this, &connection]() -> std::string { | ||
| if (upload_id_.empty()) { | ||
| upload_id_ = | ||
| connection->create_multipart_object(bucket_name_, object_key_); | ||
| } | ||
| return upload_id_; | ||
| }; |
There was a problem hiding this comment.
I think this should be a separate method. Sorry if my comment was confusing—I typically refer to accessor methods as computed properties.
src/writers/s3.sink.cpp
Outdated
| } | ||
|
|
||
| bool | ||
| zarr::S3Sink::write(size_t offset, const uint8_t* data, size_t bytes_of_data) |
There was a problem hiding this comment.
A common pattern for identifying unused parameters is using underscores.
zarr::S3Sink::write(size_t _, const uint8_t* data, size_t bytes_of_data)
src/writers/sink.creator.cpp
Outdated
| const auto object_key = object_keys.front(); | ||
| object_keys.pop(); |
There was a problem hiding this comment.
I think I’m missing something here. I’ll tinker with the code to better understand this requirement.
No description provided.