From f12e2e9ce98998d9e5087d24cc3fe45a71008458 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 7 Oct 2019 15:07:01 -0400 Subject: [PATCH 1/5] Fix InstantiatingGrpcChannelProvider's channel pool to play nicely with DirectPath By default grpclb strategy for DirectPath is to create a subchannel for every address resolved by the grpclb and round robin over them. Unfortunately this doesn't work well when Channel pooling is enable in the InstantiatingGrpcChannelProvider, which will create multiple ManagedChannels, each containing a bunch of subchannels. Since channel pooling is needed to have good performance targeting CFEs, the solution here is to force each ManagedChannel to pick a single subchannel. Thus preserving the CFE behavior of a single ManagedChannel only containing a single subchannel. --- .../grpc/InstantiatingGrpcChannelProvider.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index c958d4bcf..9d71964e2 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -42,6 +42,7 @@ import com.google.auth.oauth2.ComputeEngineCredentials; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import io.grpc.ManagedChannel; import io.grpc.ManagedChannelBuilder; import io.grpc.alts.ComputeEngineChannelBuilder; @@ -238,6 +239,21 @@ private ManagedChannel createSingleChannel() throws IOException { // Will be overridden by user defined values if any. builder.keepAliveTime(DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS, TimeUnit.SECONDS); builder.keepAliveTimeout(DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS, TimeUnit.SECONDS); + + // When channel pooling is enabled, force the pick_first grpclb strategy. + // This is necessary to avoid the multiplicative effect of creating channel pool with + // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. + if (poolSize > 1) { + builder.defaultServiceConfig( + ImmutableMap.of( + "loadBalancingConfig", + ImmutableList.of( + ImmutableMap.of( + "grpclb", + ImmutableMap.of( + "childPolicy", + ImmutableList.of(ImmutableMap.of("pick_first", ImmutableMap.of()))))))); + } } else { builder = ManagedChannelBuilder.forAddress(serviceAddress, port); } From c1c225ef6e80efceed2b3f84101bc941f792e49a Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 7 Oct 2019 16:52:55 -0400 Subject: [PATCH 2/5] Always use pick_first even if poolSize = 1 --- .../InstantiatingGrpcChannelProvider.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index 9d71964e2..6252b36b8 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -243,17 +243,15 @@ private ManagedChannel createSingleChannel() throws IOException { // When channel pooling is enabled, force the pick_first grpclb strategy. // This is necessary to avoid the multiplicative effect of creating channel pool with // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. - if (poolSize > 1) { - builder.defaultServiceConfig( - ImmutableMap.of( - "loadBalancingConfig", - ImmutableList.of( - ImmutableMap.of( - "grpclb", - ImmutableMap.of( - "childPolicy", - ImmutableList.of(ImmutableMap.of("pick_first", ImmutableMap.of()))))))); - } + builder.defaultServiceConfig( + ImmutableMap.of( + "loadBalancingConfig", + ImmutableList.of( + ImmutableMap.of( + "grpclb", + ImmutableMap.of( + "childPolicy", + ImmutableList.of(ImmutableMap.of("pick_first", ImmutableMap.of()))))))); } else { builder = ManagedChannelBuilder.forAddress(serviceAddress, port); } From 5238b9581ce3bfb043aa7b0c30949aa8639e3af0 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Wed, 9 Oct 2019 12:49:40 -0400 Subject: [PATCH 3/5] split up service config --- .../InstantiatingGrpcChannelProvider.java | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index 6252b36b8..b5c8e5f25 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -243,15 +243,19 @@ private ManagedChannel createSingleChannel() throws IOException { // When channel pooling is enabled, force the pick_first grpclb strategy. // This is necessary to avoid the multiplicative effect of creating channel pool with // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. - builder.defaultServiceConfig( - ImmutableMap.of( - "loadBalancingConfig", - ImmutableList.of( - ImmutableMap.of( - "grpclb", - ImmutableMap.of( - "childPolicy", - ImmutableList.of(ImmutableMap.of("pick_first", ImmutableMap.of()))))))); + ImmutableMap pickFirstStrategy = + ImmutableMap.of("pick_first", ImmutableMap.of()); + + ImmutableMap childPolicy = + ImmutableMap.of("childPolicy", ImmutableList.of(pickFirstStrategy)); + + ImmutableMap grpcLbPolicy = + ImmutableMap.of("grpclb", childPolicy); + + ImmutableMap loadBalancingConfig = + ImmutableMap.of("loadBalancingConfig", ImmutableList.of(grpcLbPolicy)); + + builder.defaultServiceConfig(loadBalancingConfig); } else { builder = ManagedChannelBuilder.forAddress(serviceAddress, port); } From e81579ce8d3a79f6e4a173c191d3d13b43d82e77 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Wed, 9 Oct 2019 12:51:55 -0400 Subject: [PATCH 4/5] reformat service config --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index b5c8e5f25..e471bc697 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -242,7 +242,7 @@ private ManagedChannel createSingleChannel() throws IOException { // When channel pooling is enabled, force the pick_first grpclb strategy. // This is necessary to avoid the multiplicative effect of creating channel pool with - // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. + // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. ImmutableMap pickFirstStrategy = ImmutableMap.of("pick_first", ImmutableMap.of()); From 4ed30690b72609e2316865ebafc2ddf637cc2eb6 Mon Sep 17 00:00:00 2001 From: Igor Bernstein Date: Mon, 14 Oct 2019 09:27:12 -0400 Subject: [PATCH 5/5] added a ref for the service config proto --- .../google/api/gax/grpc/InstantiatingGrpcChannelProvider.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java index e471bc697..214dd058c 100644 --- a/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java +++ b/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java @@ -243,6 +243,8 @@ private ManagedChannel createSingleChannel() throws IOException { // When channel pooling is enabled, force the pick_first grpclb strategy. // This is necessary to avoid the multiplicative effect of creating channel pool with // `poolSize` number of `ManagedChannel`s, each with a `subSetting` number of number of subchannels. + // See the service config proto definition for more details: + // https://github.com/grpc/grpc-proto/blob/master/grpc/service_config/service_config.proto#L182 ImmutableMap pickFirstStrategy = ImmutableMap.of("pick_first", ImmutableMap.of());