cloud_storage: Add download_object method for downloading small objects

[abhijat]

A download_object method is added to cloud_storage::remote to download small objects intended to remain in memory and not written to disk via a stream etc.

The upload_object_request is generalized into a transfer_request type with variants for upload and download.

The download_index method is refactored to use download_object.

The new method will be used in a follow up PR to check if an inventory configuration is present or not.

Backports Required

• none - not a bug fix
• none - this is a backport
• none - issue does not exist in previous branches
• none - papercut/not impactful enough to backport
• v23.3.x
• v23.2.x
• v23.1.x

Release Notes

• none

[abhijat]

To remove the template parameter on this request, we could use a variant here and rely on default initialization, eg

using transfer_t = std::variant<upload_type, download_type>;
transfer_t transferred_type{};

Similarly we could use a variant for the buffer type:

std::variant<iobuf, std::reference_wrapper<iobuf>>;

But now we just have the one request type, this would allow the caller to call upload_object with download_object_request, iobuf& with upload_type etc. With the template parameter and associated traits it is not possible to use the upload_object or download_object with an incorrect combination of request, iobuf type or object type.

[andrwng]

this would allow the caller to call upload_object with download_object_request

Do you have a particular use case for this in mind? IMO this is a good reason to not overload the payload, and instead encapsulate some transfer-agnostic request args (bucket, key, etc), and separately have the download or upload payload. It otherwise seems like it'd be error-prone and harder to verify code by inspection (e.g. conceptually it seems wrong to implicitly allow uploading with a download request -- would it always be a bug? if so, why allow it?)

Something like:

struct object_request_params {
    bucket_name bucket;
    object_key key;
    retry_chain_node& parent_rtc;
    ....
};

struct upload_request {
    object_request_params params;
    iobuf payload;
};

struct download_request {
    object_request_params params;
    iobuf& payload;
};

[abhijat]

My comment was ambiguous, sorry. To be clear, as the PR is implemented now, an upload request is not usable with a download object method, and a download request is similarly not usable with an upload object method. Right now the types ensure that only the right request can be used by the right method.

If there had been a simple transfer_request object, there would have been a possibility of such a mixup which we avoid right now.

That said, I like the suggestion of extracting a struct with the common bits and creating the request objects by composing that and the upload/download specific fields. It looks like it might be much simpler and easier to read, I will try it out in this PR, thanks

[abhijat]

I tried this change and the definition is much simpler:

struct transfer_object_details {
    cloud_storage_clients::bucket_name bucket;
    cloud_storage_clients::object_key key;
    retry_chain_node& parent_rtc;

    std::optional<probe_callback_t> success_cb{std::nullopt};
    std::optional<probe_callback_t> failure_cb{std::nullopt};
    std::optional<probe_callback_t> backoff_cb{std::nullopt};

    void on_success(remote_probe& probe);
    void on_failure(remote_probe& probe);
    void on_backoff(remote_probe& probe);
};

struct upload_object_request {
    transfer_object_details transfer;
    iobuf payload;
    upload_type type{upload_type::object};
};

struct download_object_request {
    transfer_object_details transfer;
    iobuf& payload;
    download_type type{download_type::object};
};

The usage is more complicated:

co_await _remote.upload_object({
          .transfer = {
            .bucket = _conf->bucket_name,
            .key = cloud_storage_clients::object_key{index_path},
            .parent_rtc = fib,
            .success_cb = [](auto& probe) { probe.index_upload(); },
            .failure_cb = [](auto& probe) { probe.failed_index_upload(); }},
          .payload = idx_res->index.to_iobuf(),
          .type = cloud_storage::upload_type::segment_index,
        });

I'd prefer the simpler usage as there are many call sites.

[andrwng]

I may be missing something, but the call sites don't seem significantly more complicated than in the other approach, compared to the complexity of the object_transfer_request?

I also still don't really understand why the types of the download and upload request should share the same class given they're doing fundamentally different things -- I get that the enum and variant gives us this distinction, but it seems weird to lump them together.

[andrwng]

Functionally I know this doesn't change anything, so I'm happy to approve if you feel strongly that keeping as is is the right approach.

[abhijat]

I think the nested initializers are harder to read at a glance than flat initializers, eg

remote.local()
        .upload_object({
          .transfer_details
          = {.bucket = bucket, .key = path, .parent_rtc = fib},
          .payload = make_iobuf_from_string("p"),
        })

vs

remote.local()
        .upload_object({.bucket = bucket, .key = path, .parent_rtc = fib,
          .payload = make_iobuf_from_string("p")
        })

But I don't feel strongly about this and readability is subjective, so I have made the changes.

[andrwng]

Thank you! Very much appreciate the accommodation.

[andijcr]

lgtm

[andijcr]

finally we can use it

[andijcr]

lgtm but maybe you want to wait for a final ok from andrew

[andijcr]

nit: it's a shame clang-format does not this automatically
if you happen to push other commits:

Suggested change

	"finalize "
	"(insync_offset={})",
	"finalize (insync_offset={})",

[abhijat]

will add this in the next PR

[dotnwat]

if you add trailing , (e.g. to_xml(cfg)},) then i think clang format may do a better job here.

[dotnwat]

right, like this.

[dotnwat]

would not have guessed this was allowed, but cool !

[abhijat]

Yeah, this is a new addition since c++20. It doesn't add value outside of such operator overloads which serve the enum class, but IMO looks a little less verbose here.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1099r5.html https://en.cppreference.com/w/cpp/language/enum#Using-enum-declaration

[dotnwat]

nice!