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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8250984
feat: update DirectPath environment variables
mohanli-ml Jun 23, 2021
d70e56b
feat: update DirectPath environment variables
mohanli-ml Jun 29, 2021
5d474e2
feat: update DirectPath environment variables
mohanli-ml Jun 29, 2021
58c01eb
Merge branch 'master' into disable-dp
vam-google Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -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).

static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH = "GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
static final long DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS = 3600;
static final long DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS = 20;
Expand Down Expand Up @@ -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.

// 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

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

for (String service : whiteList.split(",")) {
if (!service.isEmpty() && serviceAddress.contains(service)) return true;
}
return false;
}

Expand Down Expand Up @@ -304,7 +313,7 @@ private ManagedChannel createSingleChannel() throws IOException, GeneralSecurity
ManagedChannelBuilder<?> builder;

// TODO(weiranf): Add API in ComputeEngineCredentials to check default service account.
if (isDirectPathEnabled()
if (isDirectPathEnabled(serviceAddress)
&& credentials instanceof ComputeEngineCredentials
&& isOnComputeEngine()) {
builder = ComputeEngineChannelBuilder.forAddress(serviceAddress, port);
Expand Down