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.

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: update DirectPath environment variables #1412

Merged
merged 4 commits into from Jun 30, 2021

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Jun 23, 2021

Add GOOGLE_CLOUD_DISABLE_DIRECT_PATH to allow manually disable DirectPath. Currently in Bigtable and Spanner, DirectPath is attempted by default. We need a way to disable DirectPath for debugging purpose.

@mohanli-ml mohanli-ml requested review from as code owners Jun 23, 2021
@google-cla google-cla bot added the cla: yes label Jun 23, 2021
Copy link
Contributor

@elharo elharo left a comment

This needs multiple tests.

// Only check attemptDirectPath when DIRECT_PATH_ENV_DISABLE_DIRECT_PATH is not set.
if (Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_DISABLE_DIRECT_PATH))) {
return false;
}
if (attemptDirectPath != null) {
return attemptDirectPath;
}
// Only check DIRECT_PATH_ENV_VAR when attemptDirectPath is not set.
String whiteList = envProvider.getenv(DIRECT_PATH_ENV_VAR);
if (whiteList == null) return false;
Copy link
Contributor

@elharo elharo Jun 29, 2021

Choose a reason for hiding this comment

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

all ifs are multiline, per google style

Copy link
Contributor Author

@mohanli-ml mohanli-ml Jun 29, 2021

Choose a reason for hiding this comment

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

done

@@ -243,14 +244,22 @@ public ManagedChannel createSingleChannel() throws IOException {
return GrpcTransportChannel.create(outerChannel);
}

private boolean isDirectPathEnabled() {
// TODO(weiranf): Use attemptDirectPath as the only indicator once setAttemptDirectPath is adapted
Copy link
Contributor

@elharo elharo Jun 29, 2021

Choose a reason for hiding this comment

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

please file a bug for this and reference it here

Copy link
Contributor Author

@mohanli-ml mohanli-ml Jun 29, 2021

Choose a reason for hiding this comment

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

Created b/192398509 to myself.

@@ -79,6 +79,7 @@
*/
@InternalExtensionOnly
public final class InstantiatingGrpcChannelProvider implements TransportChannelProvider {
static final String DIRECT_PATH_ENV_VAR = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH";
Copy link
Contributor

@elharo elharo Jun 29, 2021

Choose a reason for hiding this comment

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

private

Copy link
Contributor

@vam-google vam-google Jun 29, 2021

Choose a reason for hiding this comment

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

The new ones should be private, I agree, but for the old ones, I guess it will technically make it a breaknig change

Copy link
Contributor Author

@mohanli-ml mohanli-ml Jun 29, 2021

Choose a reason for hiding this comment

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

I made the new one private, and keep the old one as what it is (i.e., without adding private).

@@ -246,6 +247,10 @@ public ManagedChannel createSingleChannel() throws IOException {
// TODO(weiranf): Use attemptDirectPath as the only indicator once setAttemptDirectPath is adapted
// and the env var is removed from client environment.
private boolean isDirectPathEnabled(String serviceAddress) {
// Only check attemptDirectPath when DIRECT_PATH_ENV_DISABLE_DIRECT_PATH is not set.
if (Boolean.parseBoolean(envProvider.getenv(DIRECT_PATH_ENV_DISABLE_DIRECT_PATH))) {
Copy link
Contributor

@elharo elharo Jun 29, 2021

Choose a reason for hiding this comment

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

This code is hard to follow due to the double negative. Probably better broken up into 3 separate statements.

Copy link
Contributor Author

@mohanli-ml mohanli-ml Jun 29, 2021

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@elharo elharo left a comment

The description sounds like there are two issues here that should be filed individually in the issue tracker and addressed with one PR each.

Copy link
Contributor

@vam-google vam-google left a comment

LGTM, but please address @elharo's comments first

@@ -79,6 +79,7 @@
*/
@InternalExtensionOnly
public final class InstantiatingGrpcChannelProvider implements TransportChannelProvider {
static final String DIRECT_PATH_ENV_VAR = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH";
Copy link
Contributor

@vam-google vam-google Jun 29, 2021

Choose a reason for hiding this comment

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

The new ones should be private, I agree, but for the old ones, I guess it will technically make it a breaknig change

elharo
elharo approved these changes Jun 30, 2021
@vam-google vam-google merged commit 4f63b61 into googleapis:master Jun 30, 2021
7 checks passed
codyoss pushed a commit to googleapis/google-api-go-client that referenced this issue Feb 18, 2022
Add an environment variable GOOGLE_CLOUD_DISABLE_DIRECT_PATH to disable DirectPath. We expect users to use the EnableDirectPath option to control DirectPath for most cases, and this environment variable is for special cases like running both DP and GFE traffic on the same VM. As a result, only when this env variable is explicitly set to true will DirectPath be disabled.

Same PR for Java: googleapis/gax-java#1412.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants