-
Notifications
You must be signed in to change notification settings - Fork 391
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.
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
Changes from 1 commit
99d1262
bb3f9e2
ad184a3
c60e3ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to leave the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this was a little easier/clearer, but I'm happy to go the other way. Done. |
||
// TODO(#11111): Enable on-by-default behavior. | ||
opts.set<spanner::RouteToLeaderOption>(false); | ||
if (auto e = internal::GetEnv("GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"}) { | ||
if (*e == enable) { | ||
// Change the default to "for RW/PartitionedDml transactions" | ||
// from "never". | ||
opts.unset<spanner::RouteToLeaderOption>(); // == true | ||
break; | ||
} | ||
} | ||
} | ||
#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) { | ||
|
@@ -115,6 +129,7 @@ Options DefaultOptions(Options opts) { | |
} | ||
} | ||
} | ||
#endif | ||
} | ||
|
||
return opts; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,7 +85,13 @@ TEST(Options, Defaults) { | |
EXPECT_TRUE(opts.has<SpannerBackoffPolicyOption>()); | ||
EXPECT_TRUE(opts.has<spanner_internal::SessionPoolClockOption>()); | ||
|
||
#if 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto regarding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto response. Done. |
||
// TODO(#11111): Enable 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) { | ||
|
@@ -154,14 +160,21 @@ TEST(Options, SpannerEmulatorHost) { | |
EXPECT_NE(opts.get<GrpcCredentialOption>(), nullptr); | ||
} | ||
|
||
TEST(Options, RouteToLeaderFromEnv) { | ||
TEST(Options, RouteToLeaderFromEnvOff) { | ||
testing_util::ScopedEnvironment route_to_leader_env( | ||
"GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER", "off"); | ||
auto opts = spanner_internal::DefaultOptions(); | ||
EXPECT_TRUE(opts.has<spanner::RouteToLeaderOption>()); | ||
EXPECT_FALSE(opts.get<spanner::RouteToLeaderOption>()); | ||
} | ||
|
||
TEST(Options, RouteToLeaderFromEnvOn) { | ||
testing_util::ScopedEnvironment route_to_leader_env( | ||
"GOOGLE_CLOUD_CPP_SPANNER_ROUTE_TO_LEADER", "on"); | ||
auto opts = spanner_internal::DefaultOptions(); | ||
EXPECT_FALSE(opts.has<spanner::RouteToLeaderOption>()); | ||
} | ||
|
||
TEST(Options, TracingComponentsFromEnv) { | ||
testing_util::ScopedEnvironment tracing_components_env( | ||
"GOOGLE_CLOUD_CPP_ENABLE_TRACING", "c1,c2,c3"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why no
if (route_to_leader)
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
PartitionQuery()
calls should be unconditionally routed to the leader.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a comment (here and the other places where it is not conditional). You probably won't forget that, I will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.