Skip to content

Commit 400b776

Browse files
kannanjgithubkannanjgithub
committed
Review comments, excepting the child cluster type change handling.
1 parent c4bfaa5 commit 400b776

File tree

2 files changed

+64
-29
lines changed

2 files changed

+64
-29
lines changed

‎xds/src/main/java/io/grpc/xds/CdsLoadBalancer2.java‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,10 @@
2121
import static io.grpc.xds.XdsLbPolicies.CDS_POLICY_NAME;
2222
import static io.grpc.xds.XdsLbPolicies.CLUSTER_RESOLVER_POLICY_NAME;
2323
import static io.grpc.xds.XdsLbPolicies.PRIORITY_POLICY_NAME;
24-
import static java.util.Objects.requireNonNull;
2524

2625
import com.google.errorprone.annotations.CheckReturnValue;
2726
import io.grpc.InternalLogId;
2827
import io.grpc.LoadBalancer;
29-
import io.grpc.LoadBalancerProvider;
3028
import io.grpc.LoadBalancerRegistry;
3129
import io.grpc.NameResolver;
3230
import io.grpc.Status;
@@ -47,6 +45,7 @@
4745
import java.util.Arrays;
4846
import java.util.Collections;
4947
import java.util.HashMap;
48+
import java.util.List;
5049
import java.util.Map;
5150

5251
/**
@@ -65,7 +64,7 @@ final class CdsLoadBalancer2 extends LoadBalancer {
6564

6665
CdsLoadBalancer2(Helper helper, LoadBalancerRegistry lbRegistry) {
6766
this.helper = checkNotNull(helper, "helper");
68-
this.lbRegistry = lbRegistry;
67+
this.lbRegistry = checkNotNull(lbRegistry, "lbRegistry");
6968
logger = XdsLogger.withLogId(InternalLogId.allocate("cds-lb", helper.getAuthority()));
7069
logger.log(XdsLogLevel.INFO, "Created");
7170
}
@@ -140,13 +139,12 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
140139
childLb = lbRegistry.getProvider(CLUSTER_RESOLVER_POLICY_NAME).newLoadBalancer(helper);
141140
}
142141
} else if (clusterConfig.getChildren() instanceof AggregateConfig) {
143-
LoadBalancerProvider priorityLbProvider = lbRegistry.getProvider(PRIORITY_POLICY_NAME);
144142
if (childLb == null) {
145-
childLb = priorityLbProvider.newLoadBalancer(helper);
143+
childLb = lbRegistry.getProvider(PRIORITY_POLICY_NAME).newLoadBalancer(helper);
146144
}
147145
Map<String, PriorityChildConfig> priorityChildConfigs = new HashMap<>();
148-
for (String childCluster: requireNonNull(
149-
clusterConfig.getClusterResource().prioritizedClusterNames())) {
146+
List<String> leafClusters = ((AggregateConfig) clusterConfig.getChildren()).getLeafNames();
147+
for (String childCluster: leafClusters) {
150148
priorityChildConfigs.put(childCluster,
151149
new PriorityChildConfig(
152150
GracefulSwitchLoadBalancer.createLoadBalancingPolicyConfig(
@@ -155,9 +153,7 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {
155153
false));
156154
}
157155
childConfig = new PriorityLoadBalancerProvider.PriorityLbConfig(
158-
Collections.unmodifiableMap(priorityChildConfigs),
159-
Collections.unmodifiableList(requireNonNull(
160-
clusterConfig.getClusterResource().prioritizedClusterNames())));
156+
Collections.unmodifiableMap(priorityChildConfigs), leafClusters);
161157
} else {
162158
return fail(Status.INTERNAL.withDescription(
163159
errorPrefix() + "Unexpected cluster children type: "

‎xds/src/test/java/io/grpc/xds/CdsLoadBalancer2Test.java‎

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -414,19 +414,51 @@ public void discoverAggregateCluster_createsPriorityLbPolicy() {
414414

415415
String cluster1 = "cluster-01.googleapis.com";
416416
String cluster2 = "cluster-02.googleapis.com";
417+
String cluster3 = "cluster-03.googleapis.com";
418+
String cluster4 = "cluster-04.googleapis.com";
417419
controlPlaneService.setXdsConfig(ADS_TYPE_URL_CDS, ImmutableMap.of(
418-
// CLUSTER (aggr.) -> [cluster1 (EDS), cluster2 (EDS)]
420+
// CLUSTER (aggr.) -> [cluster1 (aggr.), cluster2 (logical DNS), cluster3 (EDS)]
419421
CLUSTER, Cluster.newBuilder()
420-
.setName(CLUSTER)
421-
.setClusterType(Cluster.CustomClusterType.newBuilder()
422-
.setName("envoy.clusters.aggregate")
423-
.setTypedConfig(Any.pack(ClusterConfig.newBuilder()
424-
.addClusters(cluster1)
425-
.addClusters(cluster2)
426-
.build())))
427-
.build(),
428-
cluster1, EDS_CLUSTER.toBuilder().setName(cluster1).build(),
429-
cluster2, EDS_CLUSTER.toBuilder().setName(cluster2).build()));
422+
.setName(CLUSTER)
423+
.setClusterType(Cluster.CustomClusterType.newBuilder()
424+
.setName("envoy.clusters.aggregate")
425+
.setTypedConfig(Any.pack(ClusterConfig.newBuilder()
426+
.addClusters(cluster1)
427+
.addClusters(cluster2)
428+
.addClusters(cluster3)
429+
.build())))
430+
.setLbPolicy(Cluster.LbPolicy.RING_HASH)
431+
.build(),
432+
// cluster1 (aggr.) -> [cluster3 (EDS), cluster4 (EDS)]
433+
cluster1, Cluster.newBuilder()
434+
.setName(cluster1)
435+
.setClusterType(Cluster.CustomClusterType.newBuilder()
436+
.setName("envoy.clusters.aggregate")
437+
.setTypedConfig(Any.pack(ClusterConfig.newBuilder()
438+
.addClusters(cluster3)
439+
.addClusters(cluster4)
440+
.build())))
441+
.build(),
442+
cluster2, Cluster.newBuilder()
443+
.setName(cluster2)
444+
.setType(Cluster.DiscoveryType.LOGICAL_DNS)
445+
.setLoadAssignment(ClusterLoadAssignment.newBuilder()
446+
.addEndpoints(LocalityLbEndpoints.newBuilder()
447+
.addLbEndpoints(LbEndpoint.newBuilder()
448+
.setEndpoint(Endpoint.newBuilder()
449+
.setAddress(Address.newBuilder()
450+
.setSocketAddress(SocketAddress.newBuilder()
451+
.setAddress("dns.example.com")
452+
.setPortValue(1111)))))))
453+
.build(),
454+
cluster3, EDS_CLUSTER.toBuilder()
455+
.setName(cluster3)
456+
.setCircuitBreakers(CircuitBreakers.newBuilder()
457+
.addThresholds(CircuitBreakers.Thresholds.newBuilder()
458+
.setPriority(RoutingPriority.DEFAULT)
459+
.setMaxRequests(UInt32Value.newBuilder().setValue(100))))
460+
.build(),
461+
cluster4, EDS_CLUSTER.toBuilder().setName(cluster4).build()));
430462
startXdsDepManager();
431463

432464
verify(helper, never()).updateBalancingState(eq(ConnectivityState.TRANSIENT_FAILURE), any());
@@ -435,16 +467,23 @@ public void discoverAggregateCluster_createsPriorityLbPolicy() {
435467
assertThat(childBalancer.name).isEqualTo(PRIORITY_POLICY_NAME);
436468
PriorityLoadBalancerProvider.PriorityLbConfig childLbConfig =
437469
(PriorityLoadBalancerProvider.PriorityLbConfig) childBalancer.config;
438-
assertThat(childLbConfig.priorities).hasSize(2);
439-
assertThat(childLbConfig.priorities.get(0)).isEqualTo(cluster1);
440-
assertThat(childLbConfig.priorities.get(1)).isEqualTo(cluster2);
441-
assertThat(childLbConfig.childConfigs).hasSize(2);
442-
PriorityLoadBalancerProvider.PriorityLbConfig.PriorityChildConfig childConfig1 =
443-
childLbConfig.childConfigs.get(cluster1);
444-
assertThat(childConfig1.toString()).isEqualTo("PriorityChildConfig{childConfig="
470+
assertThat(childLbConfig.priorities).hasSize(3);
471+
assertThat(childLbConfig.priorities.get(0)).isEqualTo(cluster3);
472+
assertThat(childLbConfig.priorities.get(1)).isEqualTo(cluster4);
473+
assertThat(childLbConfig.priorities.get(2)).isEqualTo(cluster2);
474+
assertThat(childLbConfig.childConfigs).hasSize(3);
475+
PriorityLoadBalancerProvider.PriorityLbConfig.PriorityChildConfig childConfig3 =
476+
childLbConfig.childConfigs.get(cluster3);
477+
assertThat(childConfig3.toString()).isEqualTo("PriorityChildConfig{childConfig="
478+
+ "GracefulSwitchLoadBalancer.Config{childFactory=CdsLoadBalancerProvider{"
479+
+ "policy=cds_experimental, priority=5, available=true}, childConfig=CdsConfig{"
480+
+ "name=cluster-03.googleapis.com, isDynamic=false}}, ignoreReresolution=false}");
481+
PriorityLoadBalancerProvider.PriorityLbConfig.PriorityChildConfig childConfig4 =
482+
childLbConfig.childConfigs.get(cluster4);
483+
assertThat(childConfig4.toString()).isEqualTo("PriorityChildConfig{childConfig="
445484
+ "GracefulSwitchLoadBalancer.Config{childFactory=CdsLoadBalancerProvider{"
446485
+ "policy=cds_experimental, priority=5, available=true}, childConfig=CdsConfig{"
447-
+ "name=cluster-01.googleapis.com, isDynamic=false}}, ignoreReresolution=false}");
486+
+ "name=cluster-04.googleapis.com, isDynamic=false}}, ignoreReresolution=false}");
448487
PriorityLoadBalancerProvider.PriorityLbConfig.PriorityChildConfig childConfig2 =
449488
childLbConfig.childConfigs.get(cluster2);
450489
assertThat(childConfig2.toString()).isEqualTo("PriorityChildConfig{childConfig="

0 commit comments

Comments
 (0)