impl(bigtable): Table accepts Options
#9307
Conversation
product-auto-label
bot
added
the
api: bigtable
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9307 +/- ##
=======================================
Coverage 94.59% 94.60%
=======================================
Files 1470 1470
Lines 134565 134621 +56
=======================================
+ Hits 127294 127354 +60
+ Misses 7271 7267 -4
Continue to review full report at Codecov.
|
Maybe #7688?? |
D'oh. I didn't read past the title, and I assumed the scope was only call options. Not client options + call options. Thanks. |
| @@ -48,6 +48,7 @@ static_assert(std::is_copy_assignable<bigtable::Table>::value, | |||
|
|
|||
| Status Table::Apply(SingleRowMutation mut) { | |||
| if (connection_) { | |||
| google::cloud::internal::OptionsSpan span(options_); | |||
There was a problem hiding this comment.
Could/should these be outside the if (connection_) conditional?
There was a problem hiding this comment.
The spans could be outside the if conditionals, but there is no point. The code outside of the if conditionals does not care about CurrentOptions(). I'd rather have the spans inside the if blocks.
There was a problem hiding this comment.
The code outside of the if conditionals does not care about
CurrentOptions().
OK, although I'll assert that it is difficult to know that, and it may be better to be safe than sorry.
| @@ -48,6 +48,7 @@ static_assert(std::is_copy_assignable<bigtable::Table>::value, | |||
|
|
|||
| Status Table::Apply(SingleRowMutation mut) { | |||
| if (connection_) { | |||
| google::cloud::internal::OptionsSpan span(options_); | |||
There was a problem hiding this comment.
The code outside of the if conditionals does not care about
CurrentOptions().
OK, although I'll assert that it is difficult to know that, and it may be better to be safe than sorry.
Part of the work for #7688
I do not think it makes sense to add Options to the constructors that accept a
DataClient, because no options will apply to the calls.The constructor is lacking documentation. I will add it later.
This change is