fix(spanner): propagate Options through SessionPool callbacks #11344
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`BatchCreateSessions()` calls made during the `SessionPool` callback chain depend on the prevailing options to determine whether the RPC should be routed to a region leader. So, we need to ensure that the construction-time options are propagated through those callbacks by instantiating `OptionsSpan` objects when appropriate. Add similar `OptionsSpan` objects to `session_pool_integration_test.cc`, which uses "FriendForTest" access to reach into `SessionPool`, bypassing the normal span instantiation. Add unit tests to verify that the "x-goog-spanner-route-to-leader" header is now set appropriately when the `RouteToLeaderOption` is both enabled and disabled. Add `RouteToLeaderOption` to the `SessionPoolOptionList`, marking it as a valid option to pass to `spanner::MakeConnection()`. Also add a slight optimization to `SessionPool::Grow()`, which skips even trying to create sessions when `ComputeCreateCounts()` returns an empty vector.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #11344 +/- ##
==========================================
- Coverage 93.73% 93.73% -0.01%
==========================================
Files 1792 1792
Lines 160022 160058 +36
==========================================
+ Hits 149998 150030 +32
- Misses 10024 10028 +4
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
coryan
approved these changes
Apr 21, 2023
google/cloud/spanner/options.h
Outdated
Comment on lines
93
to
99
| /** | ||
| * Option for `google::cloud::Options` to, when present and false, suppress | ||
| * adding headers to distinguish requests served by the leader v/s non-leader | ||
| * region. | ||
| * | ||
| * @ingroup spanner-options | ||
| */ |
There was a problem hiding this comment.
This assumes the reader understands leader regions already, and it is not a good "brief" comment.
Suggested change
| /** | |
| * Option for `google::cloud::Options` to, when present and false, suppress | |
| * adding headers to distinguish requests served by the leader v/s non-leader | |
| * region. | |
| * | |
| * @ingroup spanner-options | |
| */ | |
| /** | |
| * Control route-to-leader-region headers. | |
| * | |
| * Unless this option is present and `false` the client library will send | |
| * headers that route the request to the leader region. | |
| * | |
| * @see https://cloud.google.com/spanner/docs/instance-configurations | |
| * for more information on multi-regional spanner instances and the role of | |
| * leader regions. | |
| * | |
| * @ingroup spanner-options | |
| */ |
There was a problem hiding this comment.
Excellent. Done. Thanks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BatchCreateSessions()calls made during theSessionPoolcallback chain depend on the prevailing options to determine whether the RPC should be routed to a region leader. So, we need to ensure that the construction-time options are propagated through those callbacks by instantiatingOptionsSpanobjects when appropriate.Add similar
OptionsSpanobjects tosession_pool_integration_test.cc, which uses "FriendForTest" access to reach intoSessionPool, bypassing the normal span instantiation.Add unit tests to verify that the "x-goog-spanner-route-to-leader" header is now set appropriately when the
RouteToLeaderOptionis both enabled and disabled.Add
RouteToLeaderOptionto theSessionPoolOptionList, marking it as a valid option to pass tospanner::MakeConnection().Also add a slight optimization to
SessionPool::Grow(), which skips even trying to create sessions whenComputeCreateCounts()returns an empty vector.This change is