feat(spanner): Enable EEF for cloud spanner when direct access is enabled#14414
feat(spanner): Enable EEF for cloud spanner when direct access is enabled#14414kinsaurralde wants to merge 2 commits intogoogleapis:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Spanner client to enable GCP fallback by default and configures a fallback period of three minutes. Feedback was provided to improve the robustness of environment variable parsing, ensuring that invalid or malformed values do not accidentally disable the fallback feature.
| isFallbackEnabled := true // Default behavior when unset | ||
| if val, ok := os.LookupEnv("GOOGLE_SPANNER_ENABLE_GCP_FALLBACK"); ok { | ||
| isFallbackEnabled, _ = strconv.ParseBool(val) | ||
| } |
There was a problem hiding this comment.
The current implementation of environment variable parsing for GOOGLE_SPANNER_ENABLE_GCP_FALLBACK will set isFallbackEnabled to false if the environment variable is set to an invalid boolean string (e.g., a typo). Since the intended default behavior is true when the variable is unset, it is more robust to preserve that default if a malformed value is provided. This ensures that the feature remains enabled unless explicitly disabled with a valid 'false' value, adhering to the principle of not altering defaults of existing stable code paths.
isFallbackEnabled := true
if val, ok := os.LookupEnv("GOOGLE_SPANNER_ENABLE_GCP_FALLBACK"); ok {
if b, err := strconv.ParseBool(val); err == nil {
isFallbackEnabled = b
}
}References
- When introducing new components with different default behaviors, avoid altering the defaults of existing, stable code paths to prevent breaking changes for backward compatibility.
| fbOpts.EnableFallback = true | ||
| fbOpts.ErrorRateThreshold = 1 | ||
| fbOpts.MinFailedCalls = 1 | ||
| fbOpts.Period = time.Minute * 3 |
There was a problem hiding this comment.
Is there any issue/discussion for deciding 3 minute specifically?
No description provided.