Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 28, 2023
+58 −18
Merged

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
99d1262
feat(spanner): add the final pieces for the RouteToLeaderOption
devbww Mar 27, 2023
bb3f9e2
Address review comments.
devbww Mar 27, 2023
ad184a3
Make `${GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER}` override any user
devbww Mar 27, 2023
c60e3ec
Add clarifying comments to unconditional RouteToLeader() calls.
devbww Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Address review comments.
  • Loading branch information
@devbww
devbww committed Mar 27, 2023
commit bb3f9e2ab39d02db4f83afa456d18a7f9883d9ae
15 changes: 1 addition & 14 deletions google/cloud/spanner/internal/defaults.cc
View file
Original file line number Diff line number Diff line change
@@ -105,8 +105,7 @@ Options DefaultOptions(Options opts) {
(std::min)(min_sessions, max_sessions_per_channel * num_channels);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (auto const* enable : {"Y", "y", "T", "t", "1", "on"}) {
@@ -118,18 +117,6 @@ Options DefaultOptions(Options opts) {
}
}
}
#else
if (auto e = internal::GetEnv("GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER")) {
for (auto const* disable : {"N", "n", "F", "f", "0", "off"}) {
if (*e == disable) {
// Change the default from "for RW/PartitionedDml transactions"
// to "never".
opts.set<spanner::RouteToLeaderOption>(false);
break;
}
}
}
#endif
}

return opts;
6 changes: 1 addition & 5 deletions google/cloud/spanner/internal/defaults_test.cc
View file
Original file line number Diff line number Diff line change
@@ -85,13 +85,9 @@ TEST(Options, Defaults) {
EXPECT_TRUE(opts.has<SpannerBackoffPolicyOption>());
EXPECT_TRUE(opts.has<spanner_internal::SessionPoolClockOption>());

#if 1
// TODO(#11111): Enable on-by-default behavior.
// TODO(#11111): Restore on-by-default behavior.
ASSERT_TRUE(opts.has<spanner::RouteToLeaderOption>());
EXPECT_FALSE(opts.get<spanner::RouteToLeaderOption>());
#else
EXPECT_FALSE(opts.has<spanner::RouteToLeaderOption>());
#endif
}

TEST(Options, AdminDefaults) {