diff --git a/xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java b/xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java index 526c18584e..e08ea0fab4 100644 --- a/xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java +++ b/xds/src/main/java/io/grpc/xds/LoadBalancerConfigFactory.java @@ -98,15 +98,14 @@ class LoadBalancerConfigFactory { * * @throws ResourceInvalidException If the {@link Cluster} has an invalid LB configuration. */ - static ImmutableMap newConfig(Cluster cluster, boolean enableLeastRequest, - boolean enableWrr, boolean enablePickFirst) + static ImmutableMap newConfig(Cluster cluster, boolean enableLeastRequest) throws ResourceInvalidException { // The new load_balancing_policy will always be used if it is set, but for backward // compatibility we will fall back to using the old lb_policy field if the new field is not set. if (cluster.hasLoadBalancingPolicy()) { try { return LoadBalancingPolicyConverter.convertToServiceConfig(cluster.getLoadBalancingPolicy(), - 0, enableWrr, enablePickFirst); + 0); } catch (MaxRecursionReachedException e) { throw new ResourceInvalidException("Maximum LB config recursion depth reached", e); } @@ -213,8 +212,7 @@ class LoadBalancerConfigFactory { * Converts a {@link LoadBalancingPolicy} object to a service config JSON object. */ private static ImmutableMap convertToServiceConfig( - LoadBalancingPolicy loadBalancingPolicy, int recursionDepth, boolean enableWrr, - boolean enablePickFirst) + LoadBalancingPolicy loadBalancingPolicy, int recursionDepth) throws ResourceInvalidException, MaxRecursionReachedException { if (recursionDepth > MAX_RECURSION) { throw new MaxRecursionReachedException(); @@ -228,20 +226,16 @@ class LoadBalancerConfigFactory { serviceConfig = convertRingHashConfig(typedConfig.unpack(RingHash.class)); } else if (typedConfig.is(WrrLocality.class)) { serviceConfig = convertWrrLocalityConfig(typedConfig.unpack(WrrLocality.class), - recursionDepth, enableWrr, enablePickFirst); + recursionDepth); } else if (typedConfig.is(RoundRobin.class)) { serviceConfig = convertRoundRobinConfig(); } else if (typedConfig.is(LeastRequest.class)) { serviceConfig = convertLeastRequestConfig(typedConfig.unpack(LeastRequest.class)); } else if (typedConfig.is(ClientSideWeightedRoundRobin.class)) { - if (enableWrr) { - serviceConfig = convertWeightedRoundRobinConfig( - typedConfig.unpack(ClientSideWeightedRoundRobin.class)); - } + serviceConfig = convertWeightedRoundRobinConfig( + typedConfig.unpack(ClientSideWeightedRoundRobin.class)); } else if (typedConfig.is(PickFirst.class)) { - if (enablePickFirst) { - serviceConfig = convertPickFirstConfig(typedConfig.unpack(PickFirst.class)); - } + serviceConfig = convertPickFirstConfig(typedConfig.unpack(PickFirst.class)); } else if (typedConfig.is(com.github.xds.type.v3.TypedStruct.class)) { serviceConfig = convertCustomConfig( typedConfig.unpack(com.github.xds.type.v3.TypedStruct.class)); @@ -310,12 +304,10 @@ class LoadBalancerConfigFactory { * Converts a wrr_locality {@link Any} configuration to service config format. */ private static ImmutableMap convertWrrLocalityConfig(WrrLocality wrrLocality, - int recursionDepth, boolean enableWrr, boolean enablePickFirst) - throws ResourceInvalidException, - MaxRecursionReachedException { + int recursionDepth) + throws ResourceInvalidException, MaxRecursionReachedException { return buildWrrLocalityConfig( - convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(), - recursionDepth + 1, enableWrr, enablePickFirst)); + convertToServiceConfig(wrrLocality.getEndpointPickingPolicy(), recursionDepth + 1)); } /** diff --git a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java index d5fe8a0ab9..1a7ce08fa5 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java @@ -141,7 +141,7 @@ class XdsClusterResource extends XdsResourceType { CdsUpdate.Builder updateBuilder = structOrError.getStruct(); ImmutableMap lbPolicyConfig = LoadBalancerConfigFactory.newConfig(cluster, - enableLeastRequest, enableWrr, enablePickFirst); + enableLeastRequest); // Validate the LB config by trying to parse it with the corresponding LB provider. LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(lbPolicyConfig); diff --git a/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java b/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java index 6a1a5d7db7..eefc62de8f 100644 --- a/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java +++ b/xds/src/main/java/io/grpc/xds/client/XdsResourceType.java @@ -52,12 +52,6 @@ public abstract class XdsResourceType { ? Boolean.parseBoolean(System.getenv("GRPC_EXPERIMENTAL_ENABLE_LEAST_REQUEST")) : Boolean.parseBoolean(System.getProperty("io.grpc.xds.experimentalEnableLeastRequest")); - @VisibleForTesting - public static boolean enableWrr = getFlag("GRPC_EXPERIMENTAL_XDS_WRR_LB", true); - - @VisibleForTesting - protected static boolean enablePickFirst = getFlag("GRPC_EXPERIMENTAL_PICKFIRST_LB_CONFIG", true); - protected static final String TYPE_URL_CLUSTER_CONFIG = "type.googleapis.com/envoy.extensions.clusters.aggregate.v3.ClusterConfig"; protected static final String TYPE_URL_TYPED_STRUCT_UDPA = diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java index 47dad474c3..753be1a9e6 100644 --- a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java +++ b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java @@ -170,22 +170,18 @@ public class GrpcXdsClientImplDataTest { private final FilterRegistry filterRegistry = FilterRegistry.getDefaultRegistry(); private boolean originalEnableRouteLookup; private boolean originalEnableLeastRequest; - private boolean originalEnableWrr; @Before public void setUp() { originalEnableRouteLookup = XdsResourceType.enableRouteLookup; originalEnableLeastRequest = XdsResourceType.enableLeastRequest; assertThat(originalEnableLeastRequest).isFalse(); - originalEnableWrr = XdsResourceType.enableWrr; - assertThat(originalEnableWrr).isTrue(); } @After public void tearDown() { XdsResourceType.enableRouteLookup = originalEnableRouteLookup; XdsResourceType.enableLeastRequest = originalEnableLeastRequest; - XdsResourceType.enableWrr = originalEnableWrr; } @Test diff --git a/xds/src/test/java/io/grpc/xds/LoadBalancerConfigFactoryTest.java b/xds/src/test/java/io/grpc/xds/LoadBalancerConfigFactoryTest.java index 09ee670ee3..e09066461c 100644 --- a/xds/src/test/java/io/grpc/xds/LoadBalancerConfigFactoryTest.java +++ b/xds/src/test/java/io/grpc/xds/LoadBalancerConfigFactoryTest.java @@ -155,14 +155,14 @@ public class LoadBalancerConfigFactoryTest { public void roundRobin() throws ResourceInvalidException { Cluster cluster = newCluster(buildWrrPolicy(ROUND_ROBIN_POLICY)); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); } @Test public void weightedRoundRobin() throws ResourceInvalidException { Cluster cluster = newCluster(buildWrrPolicy(WRR_POLICY)); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_WRR_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_WRR_CONFIG); } @Test @@ -179,22 +179,15 @@ public class LoadBalancerConfigFactoryTest { .build()) .build())); - assertResourceInvalidExceptionThrown(cluster, true, true, true, + assertResourceInvalidExceptionThrown(cluster, true, "Invalid duration in weighted round robin config"); } - @Test - public void weightedRoundRobin_fallback_roundrobin() throws ResourceInvalidException { - Cluster cluster = newCluster(buildWrrPolicy(WRR_POLICY, ROUND_ROBIN_POLICY)); - - assertThat(newLbConfig(cluster, true, false, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); - } - @Test public void roundRobin_legacy() throws ResourceInvalidException { Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.ROUND_ROBIN).build(); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); } @Test @@ -203,7 +196,7 @@ public class LoadBalancerConfigFactoryTest { .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(RING_HASH_POLICY)) .build(); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_RING_HASH_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_RING_HASH_CONFIG); } @Test @@ -213,7 +206,7 @@ public class LoadBalancerConfigFactoryTest { .setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE)) .setHashFunction(HashFunction.XX_HASH)).build(); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_RING_HASH_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_RING_HASH_CONFIG); } @Test @@ -225,7 +218,7 @@ public class LoadBalancerConfigFactoryTest { .setMaximumRingSize(UInt64Value.of(RING_HASH_MAX_RING_SIZE)) .setHashFunction(RingHash.HashFunction.MURMUR_HASH_2).build()))).build()); - assertResourceInvalidExceptionThrown(cluster, true, true, true, "Invalid ring hash function"); + assertResourceInvalidExceptionThrown(cluster, true, "Invalid ring hash function"); } @Test @@ -233,7 +226,7 @@ public class LoadBalancerConfigFactoryTest { Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.RING_HASH).setRingHashLbConfig( RingHashLbConfig.newBuilder().setHashFunction(HashFunction.MURMUR_HASH_2)).build(); - assertResourceInvalidExceptionThrown(cluster, true, true, true, "invalid ring hash function"); + assertResourceInvalidExceptionThrown(cluster, true, "invalid ring hash function"); } @Test @@ -242,7 +235,7 @@ public class LoadBalancerConfigFactoryTest { .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(LEAST_REQUEST_POLICY)) .build(); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_LEAST_REQUEST_CONFIG); } @Test @@ -254,7 +247,7 @@ public class LoadBalancerConfigFactoryTest { LeastRequestLbConfig.newBuilder() .setChoiceCount(UInt32Value.of(LEAST_REQUEST_CHOICE_COUNT))).build(); - LbConfig lbConfig = newLbConfig(cluster, true, true, true); + LbConfig lbConfig = newLbConfig(cluster, true); assertThat(lbConfig.getPolicyName()).isEqualTo("wrr_locality_experimental"); List childConfigs = ServiceConfigUtil.unwrapLoadBalancingConfigList( @@ -269,7 +262,7 @@ public class LoadBalancerConfigFactoryTest { public void leastRequest_notEnabled() { Cluster cluster = Cluster.newBuilder().setLbPolicy(LbPolicy.LEAST_REQUEST).build(); - assertResourceInvalidExceptionThrown(cluster, false, true, true, "unsupported lb policy"); + assertResourceInvalidExceptionThrown(cluster, false, "unsupported lb policy"); } @Test @@ -278,24 +271,14 @@ public class LoadBalancerConfigFactoryTest { .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY)) .build(); - assertThat(newLbConfig(cluster, true, true, true)).isEqualTo(VALID_PICK_FIRST_CONFIG); - } - - @Test - public void pickFirst_notEnabled() throws ResourceInvalidException { - Cluster cluster = Cluster.newBuilder() - .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(PICK_FIRST_POLICY)) - .build(); - - assertResourceInvalidExceptionThrown(cluster, true, true, false, "Invalid LoadBalancingPolicy"); + assertThat(newLbConfig(cluster, true)).isEqualTo(VALID_PICK_FIRST_CONFIG); } @Test public void customRootLb_providerRegistered() throws ResourceInvalidException { LoadBalancerRegistry.getDefaultRegistry().register(CUSTOM_POLICY_PROVIDER); - assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false, - true, true)).isEqualTo(VALID_CUSTOM_CONFIG); + assertThat(newLbConfig(newCluster(CUSTOM_POLICY), false)).isEqualTo(VALID_CUSTOM_CONFIG); } @Test @@ -304,7 +287,7 @@ public class LoadBalancerConfigFactoryTest { .setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder().addPolicies(CUSTOM_POLICY)) .build(); - assertResourceInvalidExceptionThrown(cluster, false, true, true, "Invalid LoadBalancingPolicy"); + assertResourceInvalidExceptionThrown(cluster, false, "Invalid LoadBalancingPolicy"); } // When a provider for the endpoint picking custom policy is available, the configuration should @@ -316,7 +299,7 @@ public class LoadBalancerConfigFactoryTest { Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() .addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build(); - assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR); + assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR); } // When a provider for the endpoint picking custom policy is available, the configuration should @@ -328,7 +311,7 @@ public class LoadBalancerConfigFactoryTest { Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() .addPolicies(buildWrrPolicy(CUSTOM_POLICY_UDPA, ROUND_ROBIN_POLICY))).build(); - assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR); + assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_CUSTOM_CONFIG_IN_WRR); } // When a provider for the custom wrr_locality child policy is NOT available, we should fall back @@ -338,7 +321,7 @@ public class LoadBalancerConfigFactoryTest { Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy(LoadBalancingPolicy.newBuilder() .addPolicies(buildWrrPolicy(CUSTOM_POLICY, ROUND_ROBIN_POLICY))).build(); - assertThat(newLbConfig(cluster, false, true, true)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); + assertThat(newLbConfig(cluster, false)).isEqualTo(VALID_ROUND_ROBIN_CONFIG); } // When a provider for the custom wrr_locality child policy is NOT available and no alternative @@ -348,7 +331,7 @@ public class LoadBalancerConfigFactoryTest { Cluster cluster = Cluster.newBuilder().setLoadBalancingPolicy( LoadBalancingPolicy.newBuilder().addPolicies(buildWrrPolicy(CUSTOM_POLICY))).build(); - assertResourceInvalidExceptionThrown(cluster, false, true, true, "Invalid LoadBalancingPolicy"); + assertResourceInvalidExceptionThrown(cluster, false, "Invalid LoadBalancingPolicy"); } @Test @@ -375,7 +358,7 @@ public class LoadBalancerConfigFactoryTest { buildWrrPolicy( ROUND_ROBIN_POLICY))))))))))))))))))).build(); - assertResourceInvalidExceptionThrown(cluster, false, true, true, + assertResourceInvalidExceptionThrown(cluster, false, "Maximum LB config recursion depth reached"); } @@ -391,18 +374,16 @@ public class LoadBalancerConfigFactoryTest { .build()))).build(); } - private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest, boolean enableWrr, - boolean enablePickFirst) + private LbConfig newLbConfig(Cluster cluster, boolean enableLeastRequest) throws ResourceInvalidException { return ServiceConfigUtil.unwrapLoadBalancingConfig( - LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest, - enableWrr, enablePickFirst)); + LoadBalancerConfigFactory.newConfig(cluster, enableLeastRequest)); } private void assertResourceInvalidExceptionThrown(Cluster cluster, boolean enableLeastRequest, - boolean enableWrr, boolean enablePickFirst, String expectedMessage) { + String expectedMessage) { try { - newLbConfig(cluster, enableLeastRequest, enableWrr, enablePickFirst); + newLbConfig(cluster, enableLeastRequest); } catch (ResourceInvalidException e) { assertThat(e).hasMessageThat().contains(expectedMessage); return;