Standalone 8/N: Implement and test ThreadPool, S3Connection{Pool}.

[aliddell]

Depends on #299.

[shlomnissan]

I have no blocking comments here. However, there is some room for improvement.

[shlomnissan]

If this function is only called within a C++ context, consider returning a C++ string instead.

[shlomnissan]

Consider removing the trivial constructor from the implementation file.

Suggested change

	uint8_t clevel;
	uint8_t shuffle;
	BloscCompressionParams();
	uint8_t clevel {1};
	uint8_t shuffle {1};
	BloscCompressionParams() = default;

I also suggest initializing the codec_id member to an empty string or making it an optional value.

[shlomnissan]

Implicit conversions are not a risk in the usual sense because the constructor requires multiple arguments. Therefore, the explicit keyword is not strictly necessary here.

[aliddell]

explicit here is probably vestigial, from an earlier version of this class

[shlomnissan]

This class is a good candidate for the PIMPL idiom. Ideally, we should hide minio from the public interface.

[shlomnissan]

Both bucket_exists and object_exists could benefit from the [[nodiscard]] attribute in their signatures. Alternatively, consider renaming them to is_bucket_exists and is_object_exists for improved clarity.

[shlomnissan]

The same comment applies regarding the preference for inline initialization over the constructor's initializer list.

[shlomnissan]

This function name is confusing. I suggest renaming it to process_jobs() if that accurately reflects its implementation.

[shlomnissan]

std::function can be costly in terms of performance. Moreover, it's common practice to use inline lambdas for scenarios like error callbacks. I recommend shifting to function objects to address these situations more efficiently.

: error_handler_ { std::move(err) }

[aliddell]

Something like this?

class ErrorCallerbacker {
public:
    ErrorCallbacker(std::unique_ptr<ZarrStream_s> stream) : stream_(stream) {}
    int operator()(const std::string& s) const { stream_->set_error(s); }
private:
    int stream_;
};

The thread pool class would need to be templated I guess to handle different function objects.

[shlomnissan]

Ignore the last part of my comment. My primary point was moving the object instead of copying it.

[shlomnissan]

A clearer way to write this might be:

const auto max_threads = std::max(std::thread::hardware_concurrency(), 1u);
n_threads = std::clamp(n_threads, 1u, max_threads);

[shlomnissan]

A similar question arises about the need to mix different locking mechanisms. I'm not an expert when it comes to the locking API, so there might be a compelling reason for this approach that I'm overlooking.