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 per-call configuration options to Client APIs #7666

Closed
dbolduc opened this issue Nov 24, 2021 · 2 comments
Closed

Add per-call configuration options to Client APIs #7666

dbolduc opened this issue Nov 24, 2021 · 2 comments
Labels
cpp: generator Issues related to the C++ micro-generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@dbolduc
Copy link
Member

dbolduc commented Nov 24, 2021

Overview

This would offer a general framework for implementing a solution to something like #4926. (In addition to timeouts, it could be used to add Metadata, serve some service specific function, etc.).

The Client calls would all gain an Options parameter:

// calls like:
Status Foo(FooRequest const& request);
// ...become:
Status Foo(FooRequest const& request, Options const& options = {});

The Connection calls would also gain an Options parameter.

Details / Open Questions

Jotting down some thoughts from a previous discussion with the team:

  • We probably don't want to default a parameter for a virtual member function in Connection.
  • We should consider making these non-pure virtual functions. This could prevent breaks of Connection mocks when a new RPC is added to a service for users that do NOT use our provided mocks + the Google Mock framework.
  • Implementing this for GA services constitutes a breaking change. (Maybe the code will compile, but the implementation details will change, making any current mocks useless).
    • We will need to add a configuration setting to the generator to enable/disable the generation of the Options parameter.
    • How and when to migrate currently GA libraries to using per-call options is an open question.

While we probably have enough direction to start implementing the API surface, I think we need a clearer picture of what a per-call option implementation looks like.

  • gRPC configuration
    • Could be applied to configure the context just before it is passed to the stub layer
    • Could be applied in our generic loop helpers (e.g. RetryLoop)
    • Could be applied as another stub decorator. We would need to think about whether this decorator takes effect before or after other decorators (e.g. auth).
    • Would these Options apply for poll and cancel operations?
  • REST configuration (currently just Storage)
    • Has storage::DownloadStallTimeoutOption and storage::TransferStallTimeoutOption
    • They are applied to the CurlClient. (So all requests with the same client have the same timeouts. Is this a problem? I don't know.)
    • They set CURLOPT_CONNECTTIMEOUT - (transfer) (download)
  • Service-specific configuration
    • Do they exist?
    • If so, I think they would be handwritten. The developer can decide how to implement them when the time comes.

Appendix

Just to give an idea of what I currently have in mind (setting deadlines) (for gRPC):

/// FILE: google/cloud/common_options.h
namespace google::cloud {
struct DeadlineOption {
  using Type = std::chrono::time_point<std::chrono::system_clock>;
};
}  // namespace

/// FILE: google/cloud/grpc_options.h
namespace google::cloud::internal {
struct GrpcSetupOption {
  using Type = std::function<void(grpc::ClientContext&)>;
};

// TODO : Whether this alters a ClientContext& or returns a new, configured
//              ClientContext can be figured out at a later date.
void ConfigureContext(grpc::ClientContext& context, Options const& opts) {
  // TODO : Which option takes precedence, and whether they are exclusive
  //        can be decided later.
  if (opts.has<GrpcSetupOption>()) {
    opts.get<GrpcSetupOption>()(context);
  } else if (opts.has<DeadlineOption>()) {
    context.set_deadline(opts.get<DeadlineOption>());
  }
}
}  // namespace
@dbolduc dbolduc added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. cpp: generator Issues related to the C++ micro-generator labels Nov 24, 2021
dbolduc added a commit that referenced this issue Nov 25, 2021
We want to add per-call Options to the Client APIs (#7666). This would be a breaking change for any GA library. This change will not get done by the next release.

If the generated bigtable admin APIs were included in the next release, they would immediately be GA. This means we would have to support a suboptimal API. There is no hurry to generate the bigtable admin APIs, so we can revert them now, and add them back once the API surface is figured out.

Note that the other new libraries added in this release (tasks, secretmanager, pubsublite) are all marked as experimental. We make no promises for maintaining experimental APIs, so it is acceptable to leave them in the repo, even if the surface might change slightly.
@coryan
Copy link
Contributor

coryan commented Nov 30, 2021

This looks like a duplicate of #7198, I will let @devjgm and @dbolduc decide which one should be closed.

@dbolduc
Copy link
Member Author

dbolduc commented Nov 30, 2021

Whoops. I did not do my homework before writing up this issue. I am going to close this one, but copy the text over to the original issue as a comment.

@dbolduc dbolduc closed this as completed Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp: generator Issues related to the C++ micro-generator type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants