feature(spanner): request priority #6103
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #6103 +/- ##
=======================================
Coverage 95.67% 95.68%
=======================================
Files 1173 1173
Lines 104827 104889 +62
=======================================
+ Hits 100298 100359 +61
- Misses 4529 4530 +1
Continue to review full report at Codecov.
|
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
Reviewed 15 of 15 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @devbww)
google/cloud/spanner/client.h
Outdated
| Transaction transaction, std::vector<SqlStatement> statements); | ||
| StatusOr<BatchDmlResult> ExecuteBatchDml(Transaction transaction, | ||
| std::vector<SqlStatement> statements, | ||
| QueryOptions const& opts = {}); |
There was a problem hiding this comment.
Do other QueryOptions settings, like the optimizer version, make sense for these DML methods?
There was a problem hiding this comment.
For ExecutePartionedDml(), yes, because it is implemented using ExecuteSqlRequest, which at least has space for optimizer_version. That said, I don't know whether optimizer_version actually applies in Partitioned DML mode.
But for ExecuteBatchDml(), no ... it is implemented using ExecuteBatchDmlRequest, which does not have space for optimizer_version.
The alternative seemed to be to introduce a new DmlOptions, with just priority, for at least ExecuteBatchDml() (and for ExecutePartitionedDml() if optimizer_version doesn't apply there). But that seemed counter to our new "options bucket" philosophy. Still, I'm happy either way (or some other way).
There was a problem hiding this comment.
I wouldn't want to introduce a new options type, but perhaps using google::cloud::Options would make sense.
Support the marking of requests with a relative priority of high, medium, or low. This acts as a hint to the Cloud Spanner scheduler, and does not actually guarantee priority or order of execution. If unspecified, the priority defaults to high. Introduce the `google::cloud::spanner::RequestPriority` enum, and add one as an optional field to `CommitOptions`, `QueryOptions`, and `ReadOptions`, with builder-pattern support where applicable. `CommitOptions` can already be applied to `Client::Commit()`, and `ReadOptions` to `Client::Read()` and `PartitionRead()`. `QueryOptions` can already be applied to `Client::ExecuteQuery()`, `ProfileQuery()`, `ExecuteDml()`, `ProfileDml()`, and `AnalyzeSql()`, but here it is also added to `Client::ExecuteBatchDml()` and `ExecutePartitionedDml()`.
Don't add `QueryOptions` to `ExecuteBatchDml()`, where only the request priority applies. Start using `google::cloud::Options` instead. Other `spanner::Client` interfaces will be migrating in that direction in the future.
devbww
force-pushed
the
rpc-priority-merge
branch
from
c2934f2 to
631f939
Compare
4 participants
Support the marking of requests with a relative priority of high,
medium, or low. This acts as a hint to the Cloud Spanner scheduler,
and does not actually guarantee priority or order of execution.
If unspecified, the priority defaults to high.
Introduce the
google::cloud::spanner::RequestPriorityenum, andadd one as an optional field to
CommitOptions,QueryOptions,and
ReadOptions, with builder-pattern support where applicable.CommitOptionscan already be applied toClient::Commit(), andReadOptionstoClient::Read()andPartitionRead().QueryOptionscan already be applied toClient::ExecuteQuery(),ProfileQuery(),ExecuteDml(),ProfileDml(), andAnalyzeSql(),but here it is also added to
Client::ExecuteBatchDml()andExecutePartitionedDml().This change is