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

cleanup(bigtable): replace CreateDefault*Client with Make*Client #7234

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Aug 27, 2021

Fixes #6307

(the only remaining tasks are now captured in #7230, #7231)


This change is Reviewable

@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Aug 27, 2021
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 27, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 305ca8f50ce5d5031c46e08e5a5eb8499460cfd7

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc dbolduc marked this pull request as ready for review August 27, 2021 19:24
@dbolduc dbolduc requested a review from a team as a code owner August 27, 2021 19:24
@codecov
Copy link

codecov bot commented Aug 27, 2021

Codecov Report

Merging #7234 (8ae9b72) into main (d8bc8ce) will decrease coverage by 0.00%.
The diff coverage is 82.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7234      +/-   ##
==========================================
- Coverage   93.79%   93.79%   -0.01%     
==========================================
  Files        1338     1338              
  Lines      115148   115112      -36     
==========================================
- Hits       108006   107968      -38     
- Misses       7142     7144       +2     
Impacted Files Coverage Δ
google/cloud/bigtable/data_client.cc 85.00% <ø> (ø)
...e/examples/bigtable_table_admin_backup_snippets.cc 9.75% <0.00%> (+0.05%) ⬆️
google/cloud/bigtable/table.h 100.00% <ø> (ø)
...ud/bigtable/tests/admin_backup_integration_test.cc 13.88% <0.00%> (+0.37%) ⬆️
...gle/cloud/bigtable/tests/admin_integration_test.cc 7.42% <0.00%> (+0.03%) ⬆️
.../bigtable/tests/instance_admin_integration_test.cc 3.73% <0.00%> (-0.03%) ⬇️
...le/internal/async_retry_unary_rpc_and_poll_test.cc 99.25% <50.00%> (-0.75%) ⬇️
...oud/bigtable/examples/bigtable_grpc_credentials.cc 77.77% <66.66%> (ø)
...oud/bigtable/internal/async_longrunning_op_test.cc 99.21% <66.66%> (-0.79%) ⬇️
...ogle/cloud/bigtable/internal/async_poll_op_test.cc 97.87% <66.66%> (-1.05%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8bc8ce...8ae9b72. Read the comment docs.

Copy link
Contributor

@devbww devbww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bunch of "signed integer overflow" errors from ConnectionRefresh.Frequent.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 8ae9b728cd3d58c48706cafab189e97df25c8fca

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc
Copy link
Member Author

dbolduc commented Aug 27, 2021

@devbww ClientOptions internally enforced a rule that the min connection refresh period <= max connection refresh period. The implementation with Options does not do that. The test has been fixed, but I will need to think about how I want to deal with this problem going forward.

I can:

  1. change the Make*Client methods to return StatusOr<*Client>, which would fail if a min is provided that is greater than a max.
  2. edit the defaulting method to enforce the rule

At the moment, I think doing 2 makes the most sense. But it will not be done in this PR.

@devjgm
Copy link
Contributor

devjgm commented Aug 27, 2021

@devbww ClientOptions internally enforced a rule that the min connection refresh period <= max connection refresh period. The implementation with Options does not do that. The test has been fixed, but I will need to think about how I want to deal with this problem going forward.

I can:

  1. change the Make*Client methods to return StatusOr<*Client>, which would fail if a min is provided that is greater than a max.
  2. edit the defaulting method to enforce the rule

At the moment, I think doing 2 makes the most sense. But it will not be done in this PR.

FWIW, there was something similar in Spanner where we needed to enforce some constraints on Options values.

// Enforces some SessionPool constraints.
auto& num_channels = opts.lookup<GrpcNumChannelsOption>();
num_channels = (std::max)(num_channels, 1);
auto& max_idle = opts.lookup<spanner::SessionPoolMaxIdleSessionsOption>();
max_idle = (std::max)(max_idle, 0);
auto& max_sessions_per_channel =
opts.lookup<spanner::SessionPoolMaxSessionsPerChannelOption>();
max_sessions_per_channel = (std::max)(max_sessions_per_channel, 1);
auto& min_sessions = opts.lookup<spanner::SessionPoolMinSessionsOption>();
min_sessions = (std::max)(min_sessions, 0);
min_sessions =
(std::min)(min_sessions, max_sessions_per_channel * num_channels);

Copy link
Contributor

@devjgm devjgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. The "after" code is shorter and reads better. Nice!

auto credentials =
grpc::CompositeChannelCredentials(channel_credentials, call_credentials);
auto instance_admin_client(cbt::MakeInstanceAdminClient(
project_id, gc::Options{}.set<gc::GrpcCredentialOption>(credentials)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the concrete types behind these autos, but consider whether std::move(credentials) would be good advice in this documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated not even touching this. I think I should implement GUAC then see how it changes this code/documentation.

@dbolduc dbolduc merged commit 3cb70ed into googleapis:main Aug 27, 2021
@dbolduc dbolduc deleted the bigtable-cleanup-prefer-make-clients branch August 27, 2021 22:20
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. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert google/cloud/bigtable to use g::c::Options
5 participants