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

Jump to bottom

xds: eds reuse priority names for the same existing locality #9287

Merged
merged 3 commits into from Jun 24, 2022
Merged

xds: eds reuse priority names for the same existing locality #9287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
660ef68
reuse priority names
YifeiZhuang Jun 17, 2022
c2583a1
fix
YifeiZhuang Jun 24, 2022
0cadf07
Merge branch 'master' of https://github.com/grpc/grpc-java into reuse…
YifeiZhuang Jun 24, 2022
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
50 changes: 44 additions & 6 deletions xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java
View file
Open in desktop
Expand Up @@ -58,9 +58,12 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -342,6 +345,8 @@ void shutdown() {
private final class EdsClusterState extends ClusterState implements EdsResourceWatcher {
@Nullable
private final String edsServiceName;
private Map<Locality, String> localityPriorityNames = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Collections.emptyMap() would make it a bit more obvious the reference is modified instead of the map itself.

int priorityNameGenId = 1;

private EdsClusterState(String name, @Nullable String edsServiceName,
@Nullable ServerInfo lrsServerInfo, @Nullable Long maxConcurrentRequests,
Expand Down Expand Up @@ -385,10 +390,10 @@ public void run() {
List<DropOverload> dropOverloads = update.dropPolicies;
List<EquivalentAddressGroup> addresses = new ArrayList<>();
Map<String, Map<Locality, Integer>> prioritizedLocalityWeights = new HashMap<>();
List<String> sortedPriorityNames = generatePriorityNames(name, localityLbEndpoints);
for (Locality locality : localityLbEndpoints.keySet()) {
LocalityLbEndpoints localityLbInfo = localityLbEndpoints.get(locality);
int priority = localityLbInfo.priority();
String priorityName = priorityName(name, priority);
String priorityName = localityPriorityNames.get(locality);
boolean discard = true;
for (LbEndpoint endpoint : localityLbInfo.endpoints()) {
if (endpoint.isHealthy()) {
Expand Down Expand Up @@ -426,23 +431,56 @@ public void run() {
logger.log(XdsLogLevel.INFO,
"Cluster {0} has no usable priority/locality/endpoint", update.clusterName);
}
List<String> priorities = new ArrayList<>(prioritizedLocalityWeights.keySet());
Collections.sort(priorities);
sortedPriorityNames.retainAll(prioritizedLocalityWeights.keySet());
Map<String, PriorityChildConfig> priorityChildConfigs =
generateEdsBasedPriorityChildConfigs(
name, edsServiceName, lrsServerInfo, maxConcurrentRequests, tlsContext,
endpointLbPolicy, lbRegistry, prioritizedLocalityWeights, dropOverloads);
status = Status.OK;
resolved = true;
result = new ClusterResolutionResult(addresses, priorityChildConfigs, priorities,
localityWeights);
result = new ClusterResolutionResult(addresses, priorityChildConfigs,
sortedPriorityNames, localityWeights);
handleEndpointResourceUpdate();
}
}

syncContext.execute(new EndpointsUpdated());
}

private List<String> generatePriorityNames(String name,
Map<Locality, LocalityLbEndpoints> localityLbEndpoints) {
TreeMap<Integer, List<Locality>> todo = new TreeMap<>();
for (Locality locality : localityLbEndpoints.keySet()) {
int priority = localityLbEndpoints.get(locality).priority();
if (!todo.containsKey(priority)) {
todo.put(priority, new ArrayList<>());
}
todo.get(priority).add(locality);
}
Map<Locality, String> newNames = new HashMap<>();
Set<String> usedNames = new HashSet<>();
List<String> ret = new ArrayList<>();
for (Integer priority: todo.keySet()) {
String foundName = "";
for (Locality locality : todo.get(priority)) {
if (localityPriorityNames.containsKey(locality)
&& usedNames.add(localityPriorityNames.get(locality))) {
foundName = localityPriorityNames.get(locality);
break;
}
}
if ("".equals(foundName)) {
foundName = String.format("%s[priority%d]", name, priorityNameGenId++);
Copy link
Member

Choose a reason for hiding this comment

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

If we only have to update tests in ClusterResolverLoadBalancerTest, let's change "priority" to "child" like we had briefly discussed. There aren't that many occurrences in ClusterResolverLoadBalancerTest to keep around this confusion.

}
for (Locality locality : todo.get(priority)) {
newNames.put(locality, foundName);
}
ret.add(foundName);
}
localityPriorityNames = newNames;
return ret;
}

@Override
public void onResourceDoesNotExist(final String resourceName) {
syncContext.execute(new Runnable() {
Expand Down
75 changes: 75 additions & 0 deletions xds/src/test/java/io/grpc/xds/ClusterResolverLoadBalancerTest.java
View file
Open in desktop
Expand Up @@ -416,6 +416,81 @@ public void onlyEdsClusters_receivedEndpoints() {
assertThat(localityWeights).containsEntry(locality3, 20);
}

@SuppressWarnings("unchecked")
private void verifyEdsPriorityNames(List<String> want,
Map<Locality, LocalityLbEndpoints>... updates) {
ClusterResolverConfig config = new ClusterResolverConfig(
Arrays.asList(edsDiscoveryMechanism2), roundRobin);
deliverLbConfig(config);
assertThat(xdsClient.watchers.keySet()).containsExactly(EDS_SERVICE_NAME2);
assertThat(childBalancers).isEmpty();

for (Map<Locality, LocalityLbEndpoints> update: updates) {
xdsClient.deliverClusterLoadAssignment(
EDS_SERVICE_NAME2,
update);
}
assertThat(childBalancers).hasSize(1);
FakeLoadBalancer childBalancer = Iterables.getOnlyElement(childBalancers);
assertThat(childBalancer.name).isEqualTo(PRIORITY_POLICY_NAME);
PriorityLbConfig priorityLbConfig = (PriorityLbConfig) childBalancer.config;
assertThat(priorityLbConfig.priorities).isEqualTo(want);
}

@Test
@SuppressWarnings("unchecked")
public void edsUpdatePriorityName_twoPriorities() {
verifyEdsPriorityNames(Arrays.asList(CLUSTER2 + "[priority1]", CLUSTER2 + "[priority2]"),
ImmutableMap.of(locality1, createEndpoints(1),
locality2, createEndpoints(2)
));
}

@Test
@SuppressWarnings("unchecked")
public void edsUpdatePriorityName_addOnePriority() {
verifyEdsPriorityNames(Arrays.asList(CLUSTER2 + "[priority2]"),
ImmutableMap.of(locality1, createEndpoints(1)),
ImmutableMap.of(locality2, createEndpoints(1)
));
}

@Test
@SuppressWarnings("unchecked")
public void edsUpdatePriorityName_swapTwoPriorities() {
verifyEdsPriorityNames(Arrays.asList(CLUSTER2 + "[priority2]", CLUSTER2 + "[priority1]",
CLUSTER2 + "[priority3]"),
ImmutableMap.of(locality1, createEndpoints(1),
locality2, createEndpoints(2),
locality3, createEndpoints(3)
),
ImmutableMap.of(locality1, createEndpoints(2),
locality2, createEndpoints(1),
locality3, createEndpoints(3))
);
}

@Test
@SuppressWarnings("unchecked")
public void edsUpdatePriorityName_mergeTwoPriorities() {
verifyEdsPriorityNames(Arrays.asList(CLUSTER2 + "[priority3]", CLUSTER2 + "[priority1]"),
ImmutableMap.of(locality1, createEndpoints(1),
locality3, createEndpoints(3),
locality2, createEndpoints(2)),
ImmutableMap.of(locality1, createEndpoints(2),
locality3, createEndpoints(1),
locality2, createEndpoints(1)
));
}

private LocalityLbEndpoints createEndpoints(int priority) {
return LocalityLbEndpoints.create(
Arrays.asList(
LbEndpoint.create(makeAddress("endpoint-addr-1"), 100, true),
LbEndpoint.create(makeAddress("endpoint-addr-2"), 100, true)),
70 /* localityWeight */, priority /* priority */);
}

@Test
public void onlyEdsClusters_resourceNeverExist_returnErrorPicker() {
ClusterResolverConfig config = new ClusterResolverConfig(
Expand Down