feat(bigtable): add explicit instance MakeDataConnection#16191
feat(bigtable): add explicit instance MakeDataConnection#16191scotthart wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new overload of MakeDataConnection that accepts a vector of InstanceResource objects to encourage explicit instance declaration during client initialization, enabling dynamic channel pooling. The previous overload has been deprecated, and examples, benchmarks, and tests have been updated accordingly. The review feedback suggests using std::move on Options objects passed to MakeDataConnection to avoid unnecessary copies, aligning with the repository's style guide.
| return bigtable::Table( | ||
| bigtable::MakeDataConnection( | ||
| {bigtable::InstanceResource(google::cloud::Project(project_id), | ||
| instance_id)}, | ||
| options), | ||
| resource); |
There was a problem hiding this comment.
The options object is passed by value to MakeDataConnection but is not used afterward in this scope. Moving it using std::move(options) avoids an unnecessary copy of the Options object, adhering to the repository style guide regarding unnecessary copies.
return bigtable::Table(
bigtable::MakeDataConnection(
{bigtable::InstanceResource(google::cloud::Project(project_id),
instance_id)},
std::move(options)),
resource);References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
| auto connection = cbt::MakeDataConnection( | ||
| {cbt::InstanceResource(google::cloud::Project(project_id), | ||
| instance_id)}, | ||
| options); |
There was a problem hiding this comment.
The options object is passed by value to MakeDataConnection but is not used afterward in this scope. Moving it using std::move(options) avoids an unnecessary copy of the Options object, adhering to the repository style guide regarding unnecessary copies.
auto connection = cbt::MakeDataConnection(
{cbt::InstanceResource(google::cloud::Project(project_id),
instance_id)},
std::move(options));References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
| return bigtable::Table( | ||
| bigtable::MakeDataConnection( | ||
| {bigtable::InstanceResource(google::cloud::Project(project_id), | ||
| instance_id)}, | ||
| options), | ||
| resource); |
There was a problem hiding this comment.
The options object is passed by value to MakeDataConnection but is not used afterward in this scope. Moving it using std::move(options) avoids an unnecessary copy of the Options object, adhering to the repository style guide regarding unnecessary copies.
return bigtable::Table(
bigtable::MakeDataConnection(
{bigtable::InstanceResource(google::cloud::Project(project_id),
instance_id)},
std::move(options)),
resource);References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
| return bigtable::Table( | ||
| bigtable::MakeDataConnection( | ||
| {bigtable::InstanceResource(google::cloud::Project(project_id), | ||
| instance_id)}, | ||
| *ud_options), | ||
| resource); |
There was a problem hiding this comment.
The *ud_options object is passed by value to MakeDataConnection but is not used afterward in this scope. Moving it using std::move(*ud_options) avoids an unnecessary copy of the Options object, adhering to the repository style guide regarding unnecessary copies.
return bigtable::Table(
bigtable::MakeDataConnection(
{bigtable::InstanceResource(google::cloud::Project(project_id),
instance_id)},
std::move(*ud_options)),
resource);References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
| data_connection_ = MakeDataConnection( | ||
| {InstanceResource(Project(project_id()), instance_id())}, options); |
There was a problem hiding this comment.
The options object is passed by value to MakeDataConnection but is not used afterward in this scope. Moving it using std::move(options) avoids an unnecessary copy of the Options object, adhering to the repository style guide regarding unnecessary copies.
| data_connection_ = MakeDataConnection( | |
| {InstanceResource(Project(project_id()), instance_id())}, options); | |
| data_connection_ = MakeDataConnection( | |
| {InstanceResource(Project(project_id()), instance_id())}, std::move(options)); |
References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16191 +/- ##
==========================================
- Coverage 92.24% 92.23% -0.01%
==========================================
Files 2265 2265
Lines 210126 210199 +73
==========================================
+ Hits 193832 193884 +52
- Misses 16294 16315 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2850839 to
f2ebcbd
Compare
No description provided.