Standalone 5/N: Implement and test ZarrStream_s #297
Conversation
…andalone-sequence-4
…andalone-sequence-4b
aliddell
changed the base branch from
standalone-sequence-4b
to
standalone-sequence-4
There was a problem hiding this comment.
I believe there's room for improvement in this PR. My main concern is the new API for creating a dimension array using the settings object for assignment and deallocation. While I'm approving this PR to avoid blocking progress, I think we should consider this comment before merging. It's a borderline issue, but worth considering.
include/acquire.zarr.h
Outdated
| * and freed with ZarrStreamSettings_destroy_dimension_array. The order in which you | ||
| * set the dimension properties in the array should match the order of the dimensions | ||
| * from slowest to fastest changing, for example, [Z, Y, X] for a 3D dataset. | ||
| */ | ||
| typedef struct ZarrStreamSettings_s |
There was a problem hiding this comment.
Are you planning to move this type to zarr.types, or is there a specific reason for keeping it here?
There was a problem hiding this comment.
I didn't want to bury the main settings struct at the bottom of zarr.types.h.
include/acquire.zarr.h
Outdated
| ZarrDataType data_type; | ||
| ZarrVersion version; | ||
| bool multiscale; | ||
| const char* store_path; /**< Path to the store. Filesystem path or S3 key prefix. */ |
There was a problem hiding this comment.
I believe a more robust API would involve passing a store object (either as part of the settings object or separately to the "create stream" function) rather than using a single argument that could have dual meanings. I previously raised this point. However, I don't consider it a critical issue that would prevent progress.
include/acquire.zarr.h
Outdated
| ZarrStatusCode ZarrStreamSettings_create_dimension_array(ZarrStreamSettings* settings, size_t dimension_count); | ||
|
|
||
| /** | ||
| * @brief Free memory for the dimension array in the Zarr stream settings struct. | ||
| * @param[in, out] settings The Zarr stream settings struct containing the dimension array to free. | ||
| */ | ||
| void ZarrStreamSettings_destroy_dimension_array(ZarrStreamSettings* settings); |
There was a problem hiding this comment.
I believe requiring the settings object for memory allocation and deallocation blurs ownership boundaries. Instead, could the function return a pointer to the dimension array, which the user can then assign to the settings object? This way, when users want to destroy it, they can simply pass that pointer. This approach would clarify ownership and simplify memory management.
There was a problem hiding this comment.
It could, but there are two parameters that are set or reset (I double checked this and realized dimension_count is not actually getting reset in ZarrStreamSettings_destroy_dimension_array though, but I'll push a fix for that).
| @@ -1,4 +1,6 @@ | |||
| add_subdirectory(logger) | |||
| add_subdirectory(streaming) | |||
There was a problem hiding this comment.
If the "logger" is used exclusively by the streaming target and not by the driver, adding its subdirectory should be placed there instead of at the top level.
src/streaming/CMakeLists.txt
Outdated
| # Install public header files | ||
| install(DIRECTORY ${CMAKE_SOURCE_DIR}/include/ | ||
| DESTINATION include | ||
| FILES_MATCHING PATTERN "*.h" |
There was a problem hiding this comment.
Why is file matching necessary? Wouldn't all files in the public include directory be considered public header files?
|
|
||
| [[nodiscard]] | ||
| std::string | ||
| trim(const char* s) |
There was a problem hiding this comment.
This utility function, unrelated to zarr streaming, can be moved to a "utilities" header file. It can be used to clean up input in various parts of the codebase.
There was a problem hiding this comment.
It's not really used anywhere else, but I can move it to zarr.common.hh/cpp in the next PR
src/streaming/zarr.stream.cpp
Outdated
| std::string trimmed = trim(settings->endpoint); | ||
| if (trimmed.empty()) { | ||
| LOG_ERROR("S3 endpoint is empty"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The emptiness check is repeated multiple times. We should extract it into a separate function that takes an untrimmed string and an error message as parameters.
if (is_empty_string(settings->endpoint, "S3 endpoint is empty")) {
return false;
}This is also a good candidate for a utility function.
| @@ -1,6 +1,8 @@ | |||
| if (${NOTEST}) | |||
| message(STATUS "Skipping test targets") | |||
| else () | |||
| add_subdirectory(unit-tests) | |||
There was a problem hiding this comment.
What other tests are we expecting to necessitate the need for a subdirectory called "unit-tests"?
|
|
||
| #include "logger.hh" | ||
|
|
||
| #define EXPECT(e, ...) \ |
There was a problem hiding this comment.
Why don't we use a testing framework like GTest to avoid this boilerplate code every time we configure tests?
| bool | ||
| is_s3_acquisition(const struct ZarrStreamSettings_s* settings) | ||
| { | ||
| return nullptr != settings->s3_settings; |
There was a problem hiding this comment.
I already mentioned this over Slack. Yoda conditions are uncommon in modern C++ codebases because the compiler and static analyzer will warn you about this. Personally, I find this form less readable and counterintuitive, as it contradicts the natural reading order.
aliddell
force-pushed
the
standalone-sequence-4
branch
from
39ec6a7 to
bda14d7
Compare
aliddell
force-pushed
the
standalone-sequence-5
branch
from
c8f5e9f to
82df0a2
Compare
Depends on #295