feat(spanner): control replicas/regions used in non-transactional reads #13031
Conversation
Add the `DirectedReadOption` to indicate which replicas or regions should be used for `Client::Read()`, `Client::ExecuteQuery()`, and `Client::ProfileQuery()` calls in read-only or single-use transactions. - The `IncludeReplicas` variant lists the replicas to try (in order) to process the request, and what to do if the list is exhausted without finding a healthy replica. - The `ExcludeReplicas` variant lists replicas that should be excluded from serving the request.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13031 +/- ##
========================================
Coverage 93.61% 93.62%
========================================
Files 2078 2079 +1
Lines 181842 182034 +192
========================================
+ Hits 170235 170427 +192
Misses 11607 11607
☔ View full report in Codecov by Sentry. |
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
| static void ToProto( | ||
| spanner::ReplicaSelection const& from, | ||
| google::spanner::v1::DirectedReadOptions::ReplicaSelection* to) { |
There was a problem hiding this comment.
Generally we prefer return types over "output arguments as pointers". Any reason to do differently here?
| static void ToProto( | |
| spanner::ReplicaSelection const& from, | |
| google::spanner::v1::DirectedReadOptions::ReplicaSelection* to) { | |
| static google::spanner::v1::DirectedReadOptions::ReplicaSelection ToProto( | |
| spanner::ReplicaSelection const& from) { |
There was a problem hiding this comment.
Changed as you suggested.
| @@ -494,6 +546,10 @@ spanner::RowStream ConnectionImpl::ReadImpl( | |||
| *std::move(params.read_options.request_tag)); | |||
| } | |||
| request->mutable_request_options()->set_transaction_tag(ctx.tag); | |||
| absl::visit(DirectedReadVisitor([&request] { | |||
| return request->mutable_directed_read_options(); | |||
There was a problem hiding this comment.
This looks less like a factory and more like just passing a pointer or reference? Ah, I see, it leaves request unmodified if the visitor does no affect request.directed_read_options().
Please consider a comment, our future selves may be as confused as I was.
| @@ -4887,6 +4930,9 @@ void RunAll(bool emulator) { | |||
| SampleBanner("spanner_set_request_tag"); | |||
| SetRequestTag(client); | |||
|
|
|||
| SampleBanner("spanner_directed_read"); | |||
There was a problem hiding this comment.
nit: at 5000+ lines, maybe it is time to split this file? It requires some planning because one cannot just remove region tags from a file...
There was a problem hiding this comment.
Yeah. I'm in two minds. This really is a tangle, with each sample largely dependent on the state left by those before it. So, any splitting doesn't really make it easier to understand. But I'll give some more thought to a better state, and how we might get there.
Add the
DirectedReadOptionto indicate which replicas or regions should be used forClient::Read(),Client::ExecuteQuery(), andClient::ProfileQuery()calls in read-only or single-use transactions.The
IncludeReplicasvariant lists the replicas to try (in order) to process the request, and what to do if the list is exhausted without finding a healthy replica.The
ExcludeReplicasvariant lists replicas that should be excluded from serving the request.This change is