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

fix(bigtable)!: pass app profile id to connection as options #9388

Merged

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jun 30, 2022

Fixes #9349

This tedious PR does two things:

  1. stores the app profile in Table::options_ instead of Table::app_profile_id_. (Note that options_ is already passed in an OptionsSpan for each RPC). Legacy uses of app_profile_id_ become app_profile_id().
  2. drops std::string const& app_profile_id from the Connection methods. We set upOptionsSpans in the Connection unit tests so we can verify that all RPCs set a request's app_profile_id field.

While this is a breaking change at HEAD, the methods are less than 2 weeks old and have not been included in a release yet. The upside of this change is that the method signature of the Connection is closer to that of the Table. And it's nice to have one less field to type.

My apologies for not validating the Table API surface before writing all the connection code.

Also, I decided not to perpetuate the if-call-options-use-call-options-else-use-conn-options logic. As @devbww pointed out earlier, the connection options are set once. They are merged into the client options. The client options are merged into the call options. So it must be the case that either the call options have a given option, or that neither the call options nor the connection options have a given option. Therefore, we should just use the call options.


This change is Reviewable

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 5c74a490e813d1db0bf1c6f424793aaebb989266

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

@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #9388 (5c74a49) into main (8b167c8) will decrease coverage by 0.00%.
The diff coverage is 99.49%.

@@            Coverage Diff             @@
##             main    #9388      +/-   ##
==========================================
- Coverage   94.67%   94.67%   -0.01%     
==========================================
  Files        1480     1480              
  Lines      135467   135506      +39     
==========================================
+ Hits       128251   128287      +36     
- Misses       7216     7219       +3     
Impacted Files Coverage Δ
google/cloud/bigtable/data_connection.cc 16.98% <0.00%> (ø)
google/cloud/bigtable/data_connection.h 100.00% <ø> (ø)
google/cloud/bigtable/mocks/mock_data_connection.h 100.00% <ø> (ø)
...le/cloud/bigtable/internal/data_connection_impl.cc 99.15% <100.00%> (+0.01%) ⬆️
...gle/cloud/bigtable/internal/data_connection_impl.h 100.00% <100.00%> (ø)
...oud/bigtable/internal/data_connection_impl_test.cc 97.02% <100.00%> (+0.12%) ⬆️
google/cloud/bigtable/table.cc 99.35% <100.00%> (-0.01%) ⬇️
google/cloud/bigtable/table.h 98.51% <100.00%> (-0.71%) ⬇️
google/cloud/bigtable/table_test.cc 100.00% <100.00%> (ø)
...loud/bigtable/internal/connection_refresh_state.cc 97.22% <0.00%> (-2.78%) ⬇️
... and 4 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 8b167c8...5c74a49. Read the comment docs.

@dbolduc dbolduc marked this pull request as ready for review June 30, 2022 01:04
@dbolduc dbolduc requested a review from a team as a code owner June 30, 2022 01:04
@dbolduc dbolduc merged commit c290d9a into googleapis:main Jun 30, 2022
@dbolduc dbolduc deleted the bigtable-app-profile-stored-in-options branch June 30, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AppProfileIdOption for bigtable Data API.
3 participants