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.

Sign up for GitHub

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: expose property in GrpcTransportChannel if it uses direct path. #3170

Merged
merged 10 commits into from
Sep 13, 2024
Merged

feat: expose property in GrpcTransportChannel if it uses direct path. #3170

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9987538
feat: expose property in GrpcTransportChannel if it uses direct path.
zhumin8 Sep 5, 2024
4939569
avoid exposing public abstract method.
zhumin8 Sep 6, 2024
14ce302
set default and test.
zhumin8 Sep 6, 2024
cffc483
Merge branch 'main' into fix-3134
zhumin8 Sep 6, 2024
6f57cb9
use is prefix instead for autovalue.
zhumin8 Sep 12, 2024
2e16a3b
remove unecessary create method, and use builder to construct Transpo…
zhumin8 Sep 12, 2024
2f29c27
Merge branch 'main' into fix-3134
zhumin8 Sep 12, 2024
8e5a9af
add clirr ignore rule.
zhumin8 Sep 12, 2024
9611d30
remove unused clirr ignore rule from gax module.
zhumin8 Sep 12, 2024
6cfed7b
add test for default value in newBuilder().
zhumin8 Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public GrpcCallContext getEmptyCallContext() {
/** The channel in use. */
abstract ManagedChannel getManagedChannel();

public abstract boolean isDirectPath();
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved

public Channel getChannel() {
return getManagedChannel();
}
Expand Down Expand Up @@ -100,7 +102,7 @@ public void close() {
}

public static Builder newBuilder() {
return new AutoValue_GrpcTransportChannel.Builder();
return new AutoValue_GrpcTransportChannel.Builder().setDirectPath(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test case for the default value here?

}

public static GrpcTransportChannel create(ManagedChannel channel) {
Expand All @@ -111,6 +113,8 @@ public static GrpcTransportChannel create(ManagedChannel channel) {
public abstract static class Builder {
public abstract Builder setManagedChannel(ManagedChannel value);

abstract Builder setDirectPath(boolean value);

public abstract GrpcTransportChannel build();
}
}
9 changes: 6 additions & 3 deletions ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,12 @@ public TransportChannel getTransportChannel() throws IOException {
}

private TransportChannel createChannel() throws IOException {
return GrpcTransportChannel.create(
ChannelPool.create(
channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel));
return GrpcTransportChannel.newBuilder()
.setManagedChannel(
ChannelPool.create(
channelPoolSettings, InstantiatingGrpcChannelProvider.this::createSingleChannel))
.setDirectPath(this.canUseDirectPath())
.build();
}

private boolean isDirectPathEnabled() {
Expand Down
20 changes: 16 additions & 4 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ void testLogDirectPathMisconfigNotOnGCE() throws Exception {
}

@Test
public void canUseDirectPath_happyPath() {
public void canUseDirectPath_happyPath() throws IOException {
System.setProperty("os.name", "Linux");
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(
Expand All @@ -718,14 +718,20 @@ public void canUseDirectPath_happyPath() {
.setAttemptDirectPath(true)
.setCredentials(computeEngineCredentials)
.setEndpoint(DEFAULT_ENDPOINT)
.setEnvProvider(envProvider);
.setEnvProvider(envProvider)
.setHeaderProvider(Mockito.mock(HeaderProvider.class));
InstantiatingGrpcChannelProvider provider =
new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016);
Truth.assertThat(provider.canUseDirectPath()).isTrue();

// verify this info is passed correctly to transport channel
TransportChannel transportChannel = provider.getTransportChannel();
Truth.assertThat(((GrpcTransportChannel) transportChannel).isDirectPath()).isTrue();
transportChannel.shutdownNow();
}

@Test
public void canUseDirectPath_directPathEnvVarDisabled() {
public void canUseDirectPath_directPathEnvVarDisabled() throws IOException {
System.setProperty("os.name", "Linux");
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(
Expand All @@ -737,10 +743,16 @@ public void canUseDirectPath_directPathEnvVarDisabled() {
.setAttemptDirectPath(true)
.setCredentials(computeEngineCredentials)
.setEndpoint(DEFAULT_ENDPOINT)
.setEnvProvider(envProvider);
.setEnvProvider(envProvider)
.setHeaderProvider(Mockito.mock(HeaderProvider.class));
InstantiatingGrpcChannelProvider provider =
new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016);
Truth.assertThat(provider.canUseDirectPath()).isFalse();

// verify this info is passed correctly to transport channel
TransportChannel transportChannel = provider.getTransportChannel();
Truth.assertThat(((GrpcTransportChannel) transportChannel).isDirectPath()).isFalse();
transportChannel.shutdownNow();
}

@Test
Expand Down
Loading