feat(spanner): add per-operation options to Commit() and Rollback() #7714
Conversation
Part of googleapis#7690 to add an `Options opts = {}` argument to `Commit()` and `Rollback()` operations. Add new `TransactionTagOption` and `CommitReturnStatsOption` types to accompany `RequestPriorityOption`. This means the caller no longer needs `CommitOptions` (although it is still used in the `Connection` layer). Run the `Client` options through `spanner_internal::DefaultOptions()`. Note that per-operation options strictly override those client-level options. Add `OptionsSpan` local variables to `Commit()` and `Rollback()` (and also `ExecuteBatchDml()`, which already took plain `Options`). Clarify which constructor and operation overloads are for backwards compatibility so that they will be easier to deprecate/remove later. Also clarify some doxygen comments about `Options` parameters. Finally, update the `Client` test and sample code to use the new options.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7714 +/- ##
==========================================
- Coverage 95.30% 95.29% -0.01%
==========================================
Files 1256 1257 +1
Lines 113697 113741 +44
==========================================
+ Hits 108355 108395 +40
- Misses 5342 5346 +4
Continue to review full report at Codecov.
|
| return Commit(mutator, std::move(rerun_policy), std::move(backoff_policy), | ||
| Options(commit_options)); | ||
| } | ||
| StatusOr<CommitResult> Commit( |
There was a problem hiding this comment.
nit: here and below, blank line before the overload?
There was a problem hiding this comment.
The idea was to group the compatibility overloads tightly within the @{...@} braces, so I would vote for leaving it out. I'm happy to be outvoted though.
| GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN | ||
|
|
||
| CommitOptions::CommitOptions(Options const& opts) | ||
| : return_stats_(opts.has<CommitReturnStatsOption>() |
There was a problem hiding this comment.
consider reimplementing CommitOptions using a single Options member variable.
There was a problem hiding this comment.
Unfortunately perhaps, I don't think the CommitOptions API allows for this as transaction_tag() returns an absl::optional<std::string> by const&, which Options doesn't provide. Perhaps one of both of those choices was a mistake, but so be it.
| * Converts to the new, recommended way to represent options of all | ||
| * varieties, `google::cloud::Options`. | ||
| */ | ||
| explicit operator Options() const; |
There was a problem hiding this comment.
Do applications need this conversion operator? If not, maybe we could use a friend function in spanner_internal?
There was a problem hiding this comment.
The thought was that this might be useful to a customer doing their own step-wise migration.
There was a problem hiding this comment.
Yes, but once introduced it cannot be removed in basically forever. Your call.
There was a problem hiding this comment.
I think that is fine, because "forever" is bounded by the lifetime of CommitOptions itself, which we should deprecate along with the other specialty spanner::*Options classes when the Options migration is complete.
|
Happy if you merge as-is. |
Part of #7690 to add an
Options opts = {}argument toCommit()and
Rollback()operations. Add newTransactionTagOptionandCommitReturnStatsOptiontypes to accompanyRequestPriorityOption.This means the caller no longer needs
CommitOptions(although it isstill used in the
Connectionlayer).Run the
Clientoptions throughspanner_internal::DefaultOptions().Note that per-operation options strictly override those client-level
options.
Add
OptionsSpanlocal variables toCommit()andRollback()(and also
ExecuteBatchDml(), which already took plainOptions).Clarify which constructor and operation overloads are for backwards
compatibility so that they will be easier to deprecate/remove later.
Also clarify some doxygen comments about
Optionsparameters.Finally, update the
Clienttest and sample code to use the newoptions.
This change is