feat(bigtable): Option to disable direct access fallback - #14193
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to disable DirectPath fallback in ClientConfigurationManager using the environment variable CBT_DISABLE_DIRECT_ACCESS_FALLBACK or system property bigtable.internal.disable-direct-access-fallback. When enabled, configurations with DirectAccessWithFallback are normalized to DirectAccessOnly. The review feedback suggests renaming the internal field disableDirectPathFallback and its associated references and test method to disableDirectAccessFallback to ensure naming consistency with the configuration keys and protobuf fields.
|
|
||
| private final ClientConfiguration defaultConfig; | ||
| private final Optional<ClientConfiguration> overrideConfig; | ||
| private final boolean disableDirectPathFallback; |
There was a problem hiding this comment.
To maintain consistency with the environment variable CBT_DISABLE_DIRECT_ACCESS_FALLBACK, the system property bigtable.internal.disable-direct-access-fallback, and the protobuf fields (DirectAccessOnly / DirectAccessWithFallback), please rename this field from disableDirectPathFallback to disableDirectAccessFallback.
| private final boolean disableDirectPathFallback; | |
| private final boolean disableDirectAccessFallback; |
| this.disableDirectPathFallback = | ||
| Optional.ofNullable(System.getenv(DISABLE_DIRECT_ACCESS_FALLBACK_ENV_VAR)) | ||
| .orElseGet( | ||
| () -> sysProps.getProperty(DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY, "false")) | ||
| .equalsIgnoreCase("true"); |
There was a problem hiding this comment.
Rename the field assignment to disableDirectAccessFallback to match the renamed field.
| this.disableDirectPathFallback = | |
| Optional.ofNullable(System.getenv(DISABLE_DIRECT_ACCESS_FALLBACK_ENV_VAR)) | |
| .orElseGet( | |
| () -> sysProps.getProperty(DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY, "false")) | |
| .equalsIgnoreCase("true"); | |
| this.disableDirectAccessFallback = | |
| Optional.ofNullable(System.getenv(DISABLE_DIRECT_ACCESS_FALLBACK_ENV_VAR)) | |
| .orElseGet( | |
| () -> sysProps.getProperty(DISABLE_DIRECT_ACCESS_FALLBACK_SYS_PROP_KEY, "false")) | |
| .equalsIgnoreCase("true"); |
| if (disableDirectPathFallback | ||
| && builder | ||
| .getSessionConfiguration() | ||
| .getChannelConfiguration() | ||
| .hasDirectAccessWithFallback()) { |
There was a problem hiding this comment.
Update the reference to use disableDirectAccessFallback instead of disableDirectPathFallback.
| if (disableDirectPathFallback | |
| && builder | |
| .getSessionConfiguration() | |
| .getChannelConfiguration() | |
| .hasDirectAccessWithFallback()) { | |
| if (disableDirectAccessFallback | |
| && builder | |
| .getSessionConfiguration() | |
| .getChannelConfiguration() | |
| .hasDirectAccessWithFallback()) { |
| } | ||
|
|
||
| @Test | ||
| void disableDirectPathFallbackTest() throws Exception { |
There was a problem hiding this comment.
Rename the test method to disableDirectAccessFallbackTest to align with the renamed field and configuration keys.
| void disableDirectPathFallbackTest() throws Exception { | |
| void disableDirectAccessFallbackTest() throws Exception { |
| private static final Logger logger = Logger.getLogger(ClientConfigurationManager.class.getName()); | ||
|
|
||
| public static final String OVERRIDE_SYS_PROP_KEY = "bigtable.internal.client-config-override"; | ||
| public static final String DISABLE_DIRECT_ACCESS_FALLBACK_ENV_VAR = |
There was a problem hiding this comment.
Do we need both env var and system property?
There was a problem hiding this comment.
I think it's okay to have both and convenient for users.
mutianf
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
nimf
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
3 participants
No description provided.