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

feat(bigtable): cheap Table creation with different resource name #8172

Merged
merged 4 commits into from
Jan 31, 2022

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jan 31, 2022

Related to #5934

We have customers that use our Table clients to interact with tables spread across various instances and projects. Because the project and instance ids are stored in the DataClient, the customer is forced to create multiple connections, which is redundant and slow. While this issue will be addressed as part of the effort to modernize the Data API, that will take time. This PR doesn't hurt, and it adds value now.

This PR stores the project and instance ids in the Table. Customers can now produce a copy of a Table with a modified resource name, without needing to make a new connection to the service.

// Old code
auto t1 = Table(MakeDataClient("project-1", "instance-A"), "table-X");
auto t2 = Table(MakeDataClient("project-2", "instance-B"), "table-Y"); // initiates an extra connection

// New code
auto t1 = Table(MakeDataClient("project-1", "instance-A"), "table-X");
auto t2 = t1.WithNewTarget("project-2", "instance-B", "table-Y"); // cheap

I think the PR is in line with our thread safety guarantees, but the reviewer should check this.

* @par Thread-safety
* Instances of this class created via copy-construction or copy-assignment
* share the underlying pool of connections. Access to these copies via multiple
* threads is guaranteed to work. Two threads operating concurrently on the same
* instance of this class is not guaranteed to work.

Should there be another version of this method that sets the app profile id as well?


This change is Reviewable

@dbolduc dbolduc requested a review from coryan January 31, 2022 04:39
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Jan 31, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 29be6a85c0dfd1f04153532f7dd2c9f537b9f0fa

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

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #8172 (033da4a) into main (2b4f74d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8172   +/-   ##
=======================================
  Coverage   94.61%   94.61%           
=======================================
  Files        1323     1323           
  Lines      118191   118256   +65     
=======================================
+ Hits       111821   111884   +63     
- Misses       6370     6372    +2     
Impacted Files Coverage Δ
...ogle/cloud/bigtable/metadata_update_policy_test.cc 100.00% <100.00%> (ø)
google/cloud/bigtable/table.h 100.00% <100.00%> (ø)
google/cloud/bigtable/table_test.cc 100.00% <100.00%> (ø)
.../cloud/storage/benchmarks/throughput_experiment.cc 74.37% <0.00%> (-0.51%) ⬇️
google/cloud/pubsub/samples/samples.cc 92.10% <0.00%> (-0.24%) ⬇️
google/cloud/examples/grpc_credential_types.cc 89.74% <0.00%> (+1.02%) ⬆️

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 2b4f74d...033da4a. Read the comment docs.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

It strikes me as odd that I can change the project and instance ids, but not the table id? That seems like a fairly uncommon use-case?

std::string const& table_id() const { return table_id_; }

/// Override the project id
void set_project_id(std::string project_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative would be:

Table WithNewTarget(std::string instance_id, std::string project_id, std::string table_id) const {
  auto table = *this;
  table.instance_id_ = std::move(instance_id);
  table.project_id_ = std::move(project_id);
  table.table_id_ = std::move(table_id);
  return table;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Thanks for the idea.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 1d15556a15207e181395af42aa55d96f438b0915

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

@dbolduc dbolduc marked this pull request as ready for review January 31, 2022 15:03
@dbolduc dbolduc requested a review from a team as a code owner January 31, 2022 15:03
@dbolduc dbolduc changed the title feat(bigtable): edit Table project and instance ids feat(bigtable): cheap Table creation with different resource name Jan 31, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: e3a9692c9359d25229d4eb18ac7863da2c7c80c3

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

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 033da4a27b66702b5da018624b97d9dfa7f981f0

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

@dbolduc dbolduc merged commit 7531fa0 into googleapis:main Jan 31, 2022
@dbolduc dbolduc deleted the bigtable-cheap-tables branch January 31, 2022 21:46
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants