Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: update DirectPath environment variables #1412

Merged
merged 4 commits into from
Jun 30, 2021
Merged

feat: update DirectPath environment variables #1412

merged 4 commits into from
Jun 30, 2021

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Jun 23, 2021 •
edited
Loading

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 a team as code owners June 23, 2021 17:34
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 23, 2021
elharo
elharo suggested changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

This needs multiple tests.

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated
// 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

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

Choose a reason for hiding this comment

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

done

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated
@@ -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

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

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.

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
@@ -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

Choose a reason for hiding this comment

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

private

Copy link
Contributor

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

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).

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java Outdated
@@ -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

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

Choose a reason for hiding this comment

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

Done

elharo
elharo suggested changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

vam-google
vam-google approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM, but please address @elharo's comments first

gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
@@ -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

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

@mohanli-ml mohanli-ml requested a review from elharo June 30, 2021 16:06
@vam-google vam-google merged commit 4f63b61 into googleapis:master Jun 30, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Jul 19, 2021
@release-please
chore: release 1.67.0 (#1425)
1e1c203 
🤖 I have created a release \*beep\* \*boop\*
---
## [1.67.0](https://www.github.com/googleapis/gax-java/compare/v1.66.0...v1.67.0) (2021-07-19)


### Features

* introduce closeAsync to Batcher ([#1423](https://www.github.com/googleapis/gax-java/issues/1423)) ([aab5288](https://www.github.com/googleapis/gax-java/commit/aab528803405c2b5f9fc89641f47abff948a876d))
* optimize unary callables to not wait for trailers ([#1356](https://www.github.com/googleapis/gax-java/issues/1356)) ([dd5f955](https://www.github.com/googleapis/gax-java/commit/dd5f955a3ab740c677fbc6f1247094798eb814a3))
* update DirectPath environment variables ([#1412](https://www.github.com/googleapis/gax-java/issues/1412)) ([4f63b61](https://www.github.com/googleapis/gax-java/commit/4f63b61f1259936aa4a1eaf9162218c787b92f2a))


### Bug Fixes

* remove `extends ApiMessage` from `HttpJsonStubCallableFactory` definition ([#1426](https://www.github.com/googleapis/gax-java/issues/1426)) ([87636a5](https://www.github.com/googleapis/gax-java/commit/87636a5812874a77e9004aab07607121efa43736))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@mohanli-ml mohanli-ml mentioned this pull request Feb 17, 2022
Merged
codyoss pushed a commit to googleapis/google-api-go-client that referenced this pull request Feb 18, 2022
@mohanli-ml
feat(transport): add an env variable to disable DirectPath (#1447)
7bce545 
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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vam-google vam-google vam-google approved these changes

@elharo elharo elharo approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mohanli-ml @elharo @vam-google