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 bigtable support for per-operation options #7688

Closed
Tracked by #8164
devbww opened this issue Dec 2, 2021 · 2 comments · Fixed by #9627
Closed
Tracked by #8164

add bigtable support for per-operation options #7688

devbww opened this issue Dec 2, 2021 · 2 comments · Fixed by #9627
Assignees
Labels
api: bigtable Issues related to the Bigtable 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.

@devbww devbww added api: bigtable Issues related to the Bigtable 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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable 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