feat(spanner): add the final pieces for the RouteToLeaderOption #11112

devbww · 2023-03-27T05:52:55Z

The final glue to add a routing header to Spanner RPCs that should be served in the leader region.

For the time being, the boolean option defaults to off (set as false), but that default can be changed with a suitably-positive value for ${GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER}. The conditional code can be removed to restore the defaults-to-on behavior. See #11111.

This change is

The final glue to add a routing header to Spanner RPCs that should be served in the leader region. For the time being, the boolean option defaults to off (set as false), but that default can be changed with a suitably-positive value for `${GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER}`. The conditional code can be removed to restore the defaults-to-on behavior. See googleapis#11111.

codecov · 2023-03-27T06:12:53Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c1c05ac) 93.78% compared to head (c60e3ec) 93.78%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11112   +/-   ##
=======================================
  Coverage   93.78%   93.78%           
=======================================
  Files        1741     1741           
  Lines      156970   156992   +22     
=======================================
+ Hits       147212   147234   +22     
  Misses       9758     9758

Impacted Files	Coverage Δ
google/cloud/spanner/internal/connection_impl.cc	`94.73% <100.00%> (-0.07%)`	⬇️
google/cloud/spanner/internal/defaults.cc	`97.53% <100.00%> (+0.09%)`	⬆️
google/cloud/spanner/internal/defaults_test.cc	`100.00% <100.00%> (ø)`
google/cloud/spanner/internal/session_pool.cc	`88.19% <100.00%> (+0.08%)`	⬆️

... and 4 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

coryan · 2023-03-27T13:33:01Z

google/cloud/spanner/internal/defaults.cc

@@ -105,6 +105,20 @@ Options DefaultOptions(Options opts) {
      (std::min)(min_sessions, max_sessions_per_channel * num_channels);

  if (!opts.has<spanner::RouteToLeaderOption>()) {
+#if 1


Do we want to leave the '#if here or just let version control store the original version?

I thought this was a little easier/clearer, but I'm happy to go the other way. Done.

coryan · 2023-03-27T13:34:59Z

google/cloud/spanner/internal/connection_impl.cc

@@ -830,6 +841,7 @@ ConnectionImpl::PartitionQueryImpl(
        Idempotency::kIdempotent,
        [&stub](grpc::ClientContext& context,
                google::spanner::v1::PartitionQueryRequest const& request) {
+          RouteToLeader(context);


Why no if (route_to_leader) here?

Because PartitionQuery() calls should be unconditionally routed to the leader.

Consider a comment (here and the other places where it is not conditional). You probably won't forget that, I will.

coryan · 2023-03-27T13:36:23Z

google/cloud/spanner/internal/defaults.cc

+#if 1
+    // TODO(#11111): Enable on-by-default behavior.
+    opts.set<spanner::RouteToLeaderOption>(false);
+    if (auto e = internal::GetEnv("GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER")) {


You have previously argued that environment variables should override the values set in the code. This does not seem to do that, thoughts?

You're correct that this environment variable is different ... it is for overriding the default value rather than a value set in code. Let me think about that and get back to you on whether that difference is a feature or a bug. Thanks.

OK ... on further thought I concur that this environment variable should follow the "env > user > default" model. It is supposed to be a "Big Red Button" for use when things go wrong (after we restore the on-by-default behavior), so the environment should have highest priority. Thanks. PTAL.

That said, there are environment variables that only supply a default value, which should be overridden by a user setting ... "user > env > default/empty/null. This very file has two: SPANNER_OPTIMIZER_VERSION and SPANNER_OPTIMIZER_STATISTICS_PACKAGE. So, we should be careful to distinguish how the environment is used in each case.

Ac. SGTM. We may want to consider some updates to our ADR regarding this, but that can happen elsewhere.

coryan · 2023-03-27T13:36:44Z

google/cloud/spanner/internal/defaults_test.cc

@@ -85,7 +85,13 @@ TEST(Options, Defaults) {
  EXPECT_TRUE(opts.has<SpannerBackoffPolicyOption>());
  EXPECT_TRUE(opts.has<spanner_internal::SessionPoolClockOption>());

+#if 1


Ditto regarding #if vs. version control.

Ditto response. Done.

setting for the `RouteToLeaderOption`, rather than giving the default value when there is no user setting.

coryan · 2023-03-28T01:19:03Z

google/cloud/spanner/internal/connection_impl.cc

@@ -830,6 +841,7 @@ ConnectionImpl::PartitionQueryImpl(
        Idempotency::kIdempotent,
        [&stub](grpc::ClientContext& context,
                google::spanner::v1::PartitionQueryRequest const& request) {
+          RouteToLeader(context);


Consider a comment (here and the other places where it is not conditional). You probably won't forget that, I will.

I've been told to include this text in the PR description: "This update contains performance optimisations that will reduce the latency of read/write transactions that originate from a region other than the default leader region." See googleapis#11112. Fixes googleapis#11111.

I've been told to include this text in the PR description: "This update contains performance optimisations that will reduce the latency of read/write transactions that originate from a region other than the default leader region." See #11112. Fixes #11111.

product-auto-label bot added the api: spanner Issues related to the Spanner API. label Mar 27, 2023

devbww marked this pull request as ready for review March 27, 2023 06:54

devbww requested a review from a team as a code owner March 27, 2023 06:54

coryan reviewed Mar 27, 2023

View reviewed changes

devbww added 2 commits March 27, 2023 12:46

Address review comments.

bb3f9e2

Make ${GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER} override any user

ad184a3

setting for the `RouteToLeaderOption`, rather than giving the default value when there is no user setting.

coryan approved these changes Mar 28, 2023

View reviewed changes

Add clarifying comments to unconditional RouteToLeader() calls.

c60e3ec

devbww merged commit bedd265 into googleapis:main Mar 28, 2023
59 checks passed

devbww deleted the route-to-leader branch March 28, 2023 06:48

devbww mentioned this pull request Aug 4, 2023

feat(spanner): enable leader aware routing by default #12319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(spanner): add the final pieces for the RouteToLeaderOption #11112

feat(spanner): add the final pieces for the RouteToLeaderOption #11112

devbww commented Mar 27, 2023 •

edited by coryan

codecov bot commented Mar 27, 2023 •

edited

coryan Mar 27, 2023

devbww Mar 27, 2023

coryan Mar 27, 2023

devbww Mar 27, 2023

coryan Mar 28, 2023

devbww Mar 28, 2023

coryan Mar 27, 2023

devbww Mar 27, 2023

devbww Mar 27, 2023

coryan Mar 28, 2023

coryan Mar 27, 2023

devbww Mar 27, 2023

coryan Mar 28, 2023

feat(spanner): add the final pieces for the RouteToLeaderOption #11112

feat(spanner): add the final pieces for the RouteToLeaderOption #11112

Conversation

devbww commented Mar 27, 2023 • edited by coryan

codecov bot commented Mar 27, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devbww commented Mar 27, 2023 •

edited by coryan

codecov bot commented Mar 27, 2023 •

edited