Skip to content
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

add storage support for per-operation options #7691

Closed
8 of 9 tasks
Tracked by #8164
devbww opened this issue Dec 2, 2021 · 5 comments · Fixed by #9247
Closed
8 of 9 tasks
Tracked by #8164

add storage support for per-operation options #7691

devbww opened this issue Dec 2, 2021 · 5 comments · Fixed by #9247
Assignees
Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@devbww
Copy link
Contributor

devbww commented Dec 2, 2021

Add an Options options = {} argument to the constructor of the client class(es). Merge these options with the default options for the service, and store them as a member of the client class.

Add an Options options = {} argument to each operation within each client. These options should then be merged with the client options from above, and installed as the prevailing options for the duration of the operation by instantiating an internal::OptionsSpan.

You could then use internal::CurrentOptions() to obtain (a const& to) the prevailing options from anywhere you might need them. Any cleanup for call paths where Options have been passed explicitly is discretionary.

Similar support for the generated client classes was added in #7683, so you might be able to use that as an example.


I (@coryan) will steal some of the description to break down the work a bit.

  • Change all the storage::internal::*Request classes to ignore google::cloud::Options in set_multiple_options()
  • Create some helpers to convert parameter packs into a google::cloud::Options and to create spans
  • Change all the member functions in storage::Client to create an OptionsSpan
  • Add tests to verify all these spans are created
  • Change storage::internal::CurlClient to use google::cloud::internal::CurrentOptions()
  • Change storage::internal::RetryClient to use google::cloud::internal::CurrentOptions()
  • Change storage::internal::GrpcClient to stop creating its own spans
  • Change storage::internal::RestClient to use google::cloud::internal::CurrentOptions()
  • Update the documentation in storage::Client
@devbww devbww added api: storage Issues related to the Cloud Storage API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Dec 2, 2021
@devbww
Copy link
Contributor Author

devbww commented Feb 16, 2022

Note: The options stored in the client class should now include the connection->options(). So something like ...

ServiceClient::ServiceClient(std::shared_ptr<ServiceConnection> connection, Options opts = {})
    : connection_(std::move(connection)),
      opts_(internal::MergeOptions(std::move(opts), 
                                   service_internal::DefaultOptions(connection_->options()))) {
  ...
}

T ServiceClient::Operation(..., Options opts = {}) {
  internal::OptionsSpan span(internal::MergeOptions(std::move(opts), opts_));
  ...
}

@devbww
Copy link
Contributor Author

devbww commented Feb 18, 2022

Addendum: Please also ensure that the OptionsSpan instantiated during ServiceClient::Operation() is saved and then restored over any work done on behalf of the operation but after Operation() returns. Typically this means for async, paginated, or streaming RPCs. (These cases may already be handled via changes in the common library.)

@coryan
Copy link
Contributor

coryan commented Feb 26, 2022

In #8462 I create an OptionSpan in storage::internal::GrpcClient function that results in some RPC. When we add per-call options, the span should be set at the storage::Client level, and that part of the PR will need to be reverted.

@coryan
Copy link
Contributor

coryan commented Jun 3, 2022

Background

All operations in the storage library already have a parameter pack:

template <typename... Options>
ObjectReadStream ReadObject(std::string const& bucket_name,
std::string const& object_name,
Options&&... options) {

There are several alternatives to add google::cloud::Options to this set.

Alternative 1 (my proposal)

Support passing google::cloud::Options o in the parameter pack. The implementation of each operation would look more or less like this:

  template <typename... Options>
  StatusOr<ObjectMetadata> GetObjectMetadata(std::string const& bucket_name,
                                             std::string const& object_name,
                                             Options&&... options) {
    internal::GetObjectMetadataRequest request(bucket_name, object_name);
    // any `options` of type `::google::cloud::Options` are ignored here.
    request.set_multiple_options(std::forward<Options>(options)...);
    OptionSpan span(MergeWithRequestOptions(options_, std::forward<Options>(options)...);
    return raw_client_->GetObjectMetadata(request);
  }

where MergeWithRequestOptions would be specific to this client class, probably implemented like this:

  google::cloud::Options MergeWithRequestOptions(g::c::Options options) { return options; }

  template <typename... Tail>
  google::cloud::Options MergeWithRequestOptions(g::c::Options options, g::c::Options per_operation, Tail&&... tail) {
    return MergeWithRequestOptions(g::c::MergeOptions(std::move(per_operation), std::move(options)), std::forward<Tail>(tail)...);
  }
  template <typename Front, typename... Tail>
  google::cloud::Options MergeWithRequestOptions(g::c::Options options, Front&& front, Tail&&... tail) {
    return MergeWithRequestOptions(std::move(options), std::forward<Tail>(tail)...);
  }

Alternative 2

Convert all things we pass in parameter packs to be FooOption classes (i.e., to define a Type). Then change all the Request classes to store data in Options. This seems fairly complicated, and it could be done after we do "Alternative 1"?

Alternative 3

Try to consume all the FooOption things directly, without the google::cloud::Options, seems very different from the other libraries, and hard to implement, and probably confusing.

@coryan
Copy link
Contributor

coryan commented Jun 8, 2022

I am going to exclude the standalone functions and postpone that work to #9211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants