From 477f0513a4db57e06713df1d186a9e04a826d85a Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 1 Aug 2023 15:01:47 -0400 Subject: [PATCH 01/25] API, Provider, but no full impl yet --- .../DeterministicSubsettingLoadBalancer.java | 155 ++++++++++++++++++ ...inisticSubsettingLoadBalancerProvider.java | 88 ++++++++++ .../services/io.grpc.LoadBalancerProvider | 1 + 3 files changed, 244 insertions(+) create mode 100644 util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java create mode 100644 util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java new file mode 100644 index 00000000000..c31b71e1b17 --- /dev/null +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -0,0 +1,155 @@ +package io.grpc.util; + +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; + +import io.grpc.EquivalentAddressGroup; +import io.grpc.Internal; +import io.grpc.LoadBalancer; +import io.grpc.Status; +import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import java.net.SocketAddress; +import java.util.ArrayList; + +/** + * Wraps a child {@code LoadBalancer}, separating the total set of backends + * into smaller subsets for the child balancer to balance across. + * + * This implements deterministic subsetting gRFC: + * https://github.com/grpc/proposal/blob/master/A68-deterministic-subsetting-lb-policy.md + */ +@Internal +public final class DeterministicSubsettingLoadBalancer extends LoadBalancer { + + private final GracefulSwitchLoadBalancer switchLb; + + @Override + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses){ + DeterministicSubsettingLoadBalancerConfig config + = (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); + + + ArrayList newAddresses = new ArrayList<>(); + for (EquivalentAddressGroup addressGroup : resolvedAddresses.getAddresses()){ + newAddresses.addAll(addressGroup.getAddresses()); + } + + switchLb.switchTo(config.childPolicy.getProvider()); + + // TODO: SUBSET + ResolvedAddresses subsetAddresses; + + switchLb.handleResolvedAddresses( + subsetAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()) + .build()); + return true; + } + + @Override + public void handleNameResolutionError(Status error) { + switchLb.handleNameResolutionError(error); + } + + @Override + public void shutdown() { + switchLb.shutdown(); + } + + public DeterministicSubsettingLoadBalancer(Helper helper){ + ChildHelper childHelper = new ChildHelper(checkNotNull(helper, "helper")); + switchLb = new GracefulSwitchLoadBalancer(childHelper); + } + + class ChildHelper extends ForwardingLoadBalancerHelper { + private Helper delegate; + + ChildHelper(Helper delegate){ + this.delegate = delegate; + } + + @Override + protected Helper delegate() { + return delegate; + } + + @Override + public Subchannel createSubchannel(CreateSubchannelArgs args) { + DeterministicSubsettingSubchannel subchannel = new DeterministicSubsettingSubchannel(delegate.createSubchannel(args)); + return subchannel; + } + } + + class DeterministicSubsettingSubchannel extends ForwardingSubchannel { + + private final Subchannel delegate; + + DeterministicSubsettingSubchannel(Subchannel delegate) { + this.delegate = delegate; + } + + @Override + protected Subchannel delegate() { + return this.delegate; + } + } + + public static final class DeterministicSubsettingLoadBalancerConfig { + + public final Integer clientIndex; + public final Integer subsetSize; + public final Boolean sortAddresses; + + public final PolicySelection childPolicy; + + private DeterministicSubsettingLoadBalancerConfig( + Integer clientIndex, + Integer subsetSize, + Boolean sortAddresses, + PolicySelection childPolicy) { + this.clientIndex = clientIndex; + this.subsetSize = subsetSize; + this.sortAddresses = sortAddresses; + this.childPolicy = childPolicy; + } + + + public static class Builder { + Integer clientIndex; // There's really no great way to set a default here. + Integer subsetSize = 10; + + Boolean sortAddresses; + PolicySelection childPolicy; + + public Builder setClientIndex (Integer clientIndex){ + checkArgument(clientIndex != null); + this.clientIndex = clientIndex; + return this; + } + + public Builder setSubsetSize (Integer subsetSize){ + checkArgument(subsetSize != null); + this.subsetSize = subsetSize; + return this; + } + + public Builder setSortAddresses (Boolean sortAddresses){ + checkArgument(sortAddresses != null); + this.sortAddresses = sortAddresses; + return this; + } + + public Builder setChildPolicy (PolicySelection childPolicy){ + checkState(childPolicy != null); + this.childPolicy = childPolicy; + return this; + } + + public DeterministicSubsettingLoadBalancerConfig build () { + checkState(childPolicy != null); + checkState(clientIndex != null); + return new DeterministicSubsettingLoadBalancerConfig(clientIndex, subsetSize, sortAddresses, childPolicy); + } + } + } +} diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java new file mode 100644 index 00000000000..a22be3cecc2 --- /dev/null +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java @@ -0,0 +1,88 @@ +package io.grpc.util; + +import io.grpc.Internal; +import io.grpc.LoadBalancer; +import io.grpc.LoadBalancerProvider; +import io.grpc.LoadBalancerRegistry; +import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; +import io.grpc.internal.JsonUtil; +import io.grpc.internal.ServiceConfigUtil; +import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import java.util.List; +import java.util.Map; +import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder; + +@Internal +public final class DeterministicSubsettingLoadBalancerProvider extends LoadBalancerProvider { + + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return new DeterministicSubsettingLoadBalancer(helper); + } + + @Override + public boolean isAvailable() { + return true; + } + + @Override + public int getPriority() { + return 5; + } + + @Override + public String getPolicyName() { + return "deterministic_subsetting"; + } + + @Override + public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { + try { + return parseLoadBalancingPolicyConfigInternal(rawConfig); + } catch (RuntimeException e) { + return ConfigOrError.fromError( + Status.UNAVAILABLE.withCause(e).withDescription( + "Failed parsing configuration for " + getPolicyName())); + } + } + + private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawConfig) { + Integer clientIndex = JsonUtil.getNumberAsInteger(rawConfig, "clientIndex"); + Integer subsetSize = JsonUtil.getNumberAsInteger(rawConfig, "subsetSize"); + Boolean sortAddresses = JsonUtil.getBoolean(rawConfig, "sortAddresses"); + + List childConfigCandidates = ServiceConfigUtil.unwrapLoadBalancingConfigList( + JsonUtil.getListOfObjects(rawConfig, "childPolicy")); + if (childConfigCandidates == null || childConfigCandidates.isEmpty()) { + return ConfigOrError.fromError(Status.INTERNAL.withDescription( + "No child policy in deterministic_subsetting LB policy " + rawConfig + )); + } + + ConfigOrError selectedConfig = + ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, + LoadBalancerRegistry.getDefaultRegistry()); + + Builder configBuilder = new Builder(); + + configBuilder.setChildPolicy((PolicySelection) selectedConfig.getConfig()); + + if (clientIndex != null) { + configBuilder.setClientIndex(clientIndex); + } else { + return ConfigOrError.fromError(Status.INTERNAL.withDescription( + "No client index set, cannot determine subsets " + rawConfig) + ); + } + + if (subsetSize != null) { + configBuilder.setSubsetSize(subsetSize); + } + + if (sortAddresses != null) { + configBuilder.setSortAddresses(sortAddresses); + } + return ConfigOrError.fromConfig(configBuilder.build()); + } +} diff --git a/util/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider b/util/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider index 1fdd69cb00b..9d36f44c511 100644 --- a/util/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider +++ b/util/src/main/resources/META-INF/services/io.grpc.LoadBalancerProvider @@ -1,2 +1,3 @@ io.grpc.util.SecretRoundRobinLoadBalancerProvider$Provider io.grpc.util.OutlierDetectionLoadBalancerProvider +io.grpc.util.DeterministicSubsettingLoadBalancerProvider From f1dd2f83a5aa5cb08c4280f508d8f9581a511efe Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 2 Aug 2023 16:31:36 -0400 Subject: [PATCH 02/25] subset implementation --- .../DeterministicSubsettingLoadBalancer.java | 72 +++++++++++++++++-- 1 file changed, 67 insertions(+), 5 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index c31b71e1b17..a51614e5441 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -11,6 +11,11 @@ import io.grpc.internal.ServiceConfigUtil.PolicySelection; import java.net.SocketAddress; import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; +import java.util.Random; +import java.util.random.RandomGenerator; /** * Wraps a child {@code LoadBalancer}, separating the total set of backends @@ -29,16 +34,15 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses){ DeterministicSubsettingLoadBalancerConfig config = (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); - - ArrayList newAddresses = new ArrayList<>(); + // The map should only retain entries for addresses in this latest update. + ArrayList allAddresses = new ArrayList<>(); for (EquivalentAddressGroup addressGroup : resolvedAddresses.getAddresses()){ - newAddresses.addAll(addressGroup.getAddresses()); + allAddresses.addAll(addressGroup.getAddresses()); } switchLb.switchTo(config.childPolicy.getProvider()); - // TODO: SUBSET - ResolvedAddresses subsetAddresses; + ResolvedAddresses subsetAddresses = buildSubsets(resolvedAddresses, config); switchLb.handleResolvedAddresses( subsetAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()) @@ -46,6 +50,56 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses){ return true; } + // implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/383 + private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, DeterministicSubsettingLoadBalancerConfig config){ + // The map should only retain entries for addresses in this latest update. + ArrayList addresses = new ArrayList<>(); + for (EquivalentAddressGroup addressGroup : allAddresses.getAddresses()){ + addresses.addAll(addressGroup.getAddresses()); + } + + if (addresses.size() <= config.subsetSize) return allAddresses; + if (config.sortAddresses) { + // if we sort, we do so via destination hashcode. this is deterministic but differs from the method used in golang. + addresses.sort(new AddressComparator()); + } + + Integer backendCount = addresses.size(); + Integer subsetCount = backendCount / config.subsetSize; + + Integer round = config.clientIndex / subsetCount; + + Integer excludedCount = backendCount % config.subsetSize; + Integer excludedStart = (round * excludedCount) % backendCount; + Integer excludedEnd = (excludedStart + excludedCount) % backendCount; + if (excludedStart <= excludedEnd) { + List subList = addresses.subList(0, excludedStart); + subList.addAll(addresses.subList(excludedEnd, backendCount-1)); + addresses = new ArrayList(subList); + } else { + addresses = new ArrayList(addresses.subList(excludedEnd, excludedStart)); + } + + Random r = new Random(round); + Collections.shuffle(addresses, r); + + Integer subsetId = config.clientIndex % subsetCount; + + Integer start = subsetId * config.subsetSize; + Integer end = start + config.subsetSize; + + List subset = addresses.subList(start, end); + + // TODO: there is most certainly a cleaner way of doing this that retains the address groups + // one idea is that we return the full resolved list, but only connect to the relevant ones and disconnect + // to the others. + ArrayList list = new ArrayList<>(); + list.add(new EquivalentAddressGroup(subset)); + + ResolvedAddresses.Builder builder = allAddresses.toBuilder(); + return builder.setAddresses(list).build(); + } + @Override public void handleNameResolutionError(Status error) { switchLb.handleNameResolutionError(error); @@ -80,6 +134,14 @@ public Subchannel createSubchannel(CreateSubchannelArgs args) { } } + class AddressComparator implements Comparator { + @Override + public int compare(SocketAddress o1, SocketAddress o2){ + return o1.hashCode() - o2.hashCode(); + } + + } + class DeterministicSubsettingSubchannel extends ForwardingSubchannel { private final Subchannel delegate; From 4058b5b6752dae41a7547c21acfe0d67b95de1fc Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Thu, 3 Aug 2023 12:43:02 -0400 Subject: [PATCH 03/25] provider test --- ...ticSubsettingLoadBalancerProviderTest.java | 87 +++++++++++++++++++ ...terministicSubsettingLoadBalancerTest.java | 2 + 2 files changed, 89 insertions(+) create mode 100644 util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java create mode 100644 util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java new file mode 100644 index 00000000000..0daac8d0cb0 --- /dev/null +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -0,0 +1,87 @@ +package io.grpc.util; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; + +import io.grpc.InternalServiceProviders; +import io.grpc.LoadBalancer.Helper; +import io.grpc.LoadBalancerProvider; +import io.grpc.NameResolver.ConfigOrError; +import io.grpc.internal.JsonParser; +import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Map; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class DeterministicSubsettingLoadBalancerProviderTest { + + private final DeterministicSubsettingLoadBalancerProvider provider = new DeterministicSubsettingLoadBalancerProvider(); + @Test + public void registered() { + for (LoadBalancerProvider current : InternalServiceProviders.getCandidatesViaServiceLoader( + LoadBalancerProvider.class, getClass().getClassLoader())) { + if (current instanceof DeterministicSubsettingLoadBalancerProvider) { + return; + } + } + fail("DeterministicSubsettingLoadBalancerProvider not registered"); + } + + @Test + public void providesLoadBalancer() { + Helper helper = mock(Helper.class); + assertThat(provider.newLoadBalancer(helper)).isInstanceOf(DeterministicSubsettingLoadBalancer.class); + } + + @Test + public void parseConfigRequiresClientIdx() throws IOException { + String lbConfig = + "{ \"clientIndex\" : null }"; + String lbConfig2 = + "{ \"clientIndex\" : -1 }"; + ArrayList configs = new ArrayList<>(); + configs.add(lbConfig); + configs.add(lbConfig2); + + ConfigOrError configOrError; + for (String config : configs) { + configOrError = null; + try { + configOrError = provider.parseLoadBalancingPolicyConfig((Map) JsonParser.parse(config)); + } catch (IOException e) { + // if JsonParser.parse fails + throw new IOException(e); + } + assertThat(configOrError.getError()).isNotNull(); + } + } + + @Test + public void parseConfigWithDefaults() throws IOException { + String lbConfig = + "{ \"clientIndex\" : 0, " + + "\"childPolicy\" : [{\"round_robin\" : {}}], " + + "\"sortAddresses\" : false }"; + ConfigOrError configOrError = null; + try { + configOrError = provider.parseLoadBalancingPolicyConfig((Map) JsonParser.parse(lbConfig)); + } catch (IOException e) { + // if JsonParser.parse fails + throw new IOException(e); + } + System.out.println(configOrError); + assertThat(configOrError.getConfig()).isNotNull(); + DeterministicSubsettingLoadBalancerConfig config = (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); + + assertThat(config.clientIndex).isEqualTo(0); + assertThat(config.sortAddresses).isEqualTo(false); + assertThat(config.childPolicy.getProvider().getPolicyName()).isEqualTo("round_robin"); + + assertThat(config.subsetSize).isEqualTo(10); + } +} diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java new file mode 100644 index 00000000000..c88bd5a9950 --- /dev/null +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -0,0 +1,2 @@ +package io.grpc.util;public class DeterministicSubsettingLoadBalancerTest { +} From 5782386660fb8451836b010ea29cc53f2ef3bdc0 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Thu, 3 Aug 2023 13:44:48 -0400 Subject: [PATCH 04/25] suppresswarnings --- ...ticSubsettingLoadBalancerProviderTest.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index 0daac8d0cb0..b739b1a5432 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -50,13 +50,7 @@ public void parseConfigRequiresClientIdx() throws IOException { ConfigOrError configOrError; for (String config : configs) { - configOrError = null; - try { - configOrError = provider.parseLoadBalancingPolicyConfig((Map) JsonParser.parse(config)); - } catch (IOException e) { - // if JsonParser.parse fails - throw new IOException(e); - } + configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(config)); assertThat(configOrError.getError()).isNotNull(); } } @@ -67,13 +61,7 @@ public void parseConfigWithDefaults() throws IOException { "{ \"clientIndex\" : 0, " + "\"childPolicy\" : [{\"round_robin\" : {}}], " + "\"sortAddresses\" : false }"; - ConfigOrError configOrError = null; - try { - configOrError = provider.parseLoadBalancingPolicyConfig((Map) JsonParser.parse(lbConfig)); - } catch (IOException e) { - // if JsonParser.parse fails - throw new IOException(e); - } + ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); System.out.println(configOrError); assertThat(configOrError.getConfig()).isNotNull(); DeterministicSubsettingLoadBalancerConfig config = (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); @@ -84,4 +72,9 @@ public void parseConfigWithDefaults() throws IOException { assertThat(config.subsetSize).isEqualTo(10); } + + @SuppressWarnings("unchecked") + private static Map parseJsonObject(String json) throws IOException { + return (Map) JsonParser.parse(json); + } } From 2276082f051f70fe401e55d7a6112bb3925a3410 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Fri, 4 Aug 2023 12:10:07 -0400 Subject: [PATCH 05/25] LBRT --- api/src/test/java/io/grpc/LoadBalancerRegistryTest.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java index 3debc871121..f44206495ad 100644 --- a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java @@ -56,7 +56,13 @@ public void stockProviders() { "outlier_detection_experimental"); assertThat(outlierDetection.getClass().getName()).isEqualTo( "io.grpc.util.OutlierDetectionLoadBalancerProvider"); - assertThat(roundRobin.getPriority()).isEqualTo(5); + assertThat(outlierDetection.getPriority()).isEqualTo(5); + + LoadBalancerProvider deterministicSubsetting = defaultRegistry.getProvider( + "deterministic_subsetting"); + assertThat(deterministicSubsetting.getClass().getName()).isEqualTo( + "io.grpc.util.DeterministicSubsettingLoadBalancerProvider"); + assertThat(deterministicSubsetting.getPriority()).isEqualTo(5); LoadBalancerProvider grpclb = defaultRegistry.getProvider("grpclb"); assertThat(grpclb).isInstanceOf(GrpclbLoadBalancerProvider.class); From 10fdbac62d53e2ad312fd38d0d2e10e51c8fb6a5 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Thu, 10 Aug 2023 14:17:46 -0400 Subject: [PATCH 06/25] testing --- .../DeterministicSubsettingLoadBalancer.java | 43 ++- ...terministicSubsettingLoadBalancerTest.java | 309 +++++++++++++++++- .../OutlierDetectionLoadBalancerTest.java | 2 +- 3 files changed, 337 insertions(+), 17 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index a51614e5441..37e2ce580c6 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -4,6 +4,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.common.collect.Lists; import io.grpc.EquivalentAddressGroup; import io.grpc.Internal; import io.grpc.LoadBalancer; @@ -15,7 +16,6 @@ import java.util.Comparator; import java.util.List; import java.util.Random; -import java.util.random.RandomGenerator; /** * Wraps a child {@code LoadBalancer}, separating the total set of backends @@ -60,7 +60,7 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini if (addresses.size() <= config.subsetSize) return allAddresses; if (config.sortAddresses) { - // if we sort, we do so via destination hashcode. this is deterministic but differs from the method used in golang. + // If we sort, we do so via destination hashcode. This is deterministic but differs from the goland instrumentation. addresses.sort(new AddressComparator()); } @@ -68,16 +68,16 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini Integer subsetCount = backendCount / config.subsetSize; Integer round = config.clientIndex / subsetCount; - + Integer excludedCount = backendCount % config.subsetSize; Integer excludedStart = (round * excludedCount) % backendCount; Integer excludedEnd = (excludedStart + excludedCount) % backendCount; if (excludedStart <= excludedEnd) { - List subList = addresses.subList(0, excludedStart); - subList.addAll(addresses.subList(excludedEnd, backendCount-1)); - addresses = new ArrayList(subList); + List subList = addresses.subList(0, excludedStart); + subList.addAll(addresses.subList(excludedEnd, backendCount)); + addresses = new ArrayList<>(subList); } else { - addresses = new ArrayList(addresses.subList(excludedEnd, excludedStart)); + addresses = new ArrayList<>(addresses.subList(excludedEnd, excludedStart)); } Random r = new Random(round); @@ -90,14 +90,22 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini List subset = addresses.subList(start, end); - // TODO: there is most certainly a cleaner way of doing this that retains the address groups - // one idea is that we return the full resolved list, but only connect to the relevant ones and disconnect - // to the others. - ArrayList list = new ArrayList<>(); - list.add(new EquivalentAddressGroup(subset)); + ArrayList eaglist = new ArrayList<>(); + + for (EquivalentAddressGroup eag : allAddresses.getAddresses()) { + List addrs = Lists.newArrayList(); + for (SocketAddress addr :eag.getAddresses()) { + if (subset.contains(addr)) { + addrs.add(addr); + } + } + if (addrs.size() != 0) { + eaglist.add(new EquivalentAddressGroup(addrs)); + } + } ResolvedAddresses.Builder builder = allAddresses.toBuilder(); - return builder.setAddresses(list).build(); + return builder.setAddresses(eaglist).build(); } @Override @@ -177,20 +185,25 @@ private DeterministicSubsettingLoadBalancerConfig( public static class Builder { - Integer clientIndex; // There's really no great way to set a default here. + Integer clientIndex; Integer subsetSize = 10; Boolean sortAddresses; PolicySelection childPolicy; public Builder setClientIndex (Integer clientIndex){ - checkArgument(clientIndex != null); + checkState(clientIndex != null); + // Indices must be positive integers. + checkState(clientIndex >= 0); this.clientIndex = clientIndex; return this; } public Builder setSubsetSize (Integer subsetSize){ checkArgument(subsetSize != null); + // subsetSize of 1 is equivalent to `pick_first`. Use that policy if that behavior is desired. + // Fallback to default of 10 of condition is not satisfied. + checkArgument(subsetSize > 1); this.subsetSize = subsetSize; return this; } diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index c88bd5a9950..e75760fe631 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -1,2 +1,309 @@ -package io.grpc.util;public class DeterministicSubsettingLoadBalancerTest { +package io.grpc.util; + + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import io.grpc.ConnectivityState; +import io.grpc.ConnectivityStateInfo; +import io.grpc.EquivalentAddressGroup; +import io.grpc.LoadBalancer; +import io.grpc.LoadBalancer.Subchannel; +import io.grpc.LoadBalancer.ResolvedAddresses; +import io.grpc.LoadBalancerProvider; +import io.grpc.internal.ServiceConfigUtil.PolicySelection; +import io.grpc.internal.TestUtils; +import java.net.SocketAddress; +import io.grpc.util.OutlierDetectionLoadBalancerTest.FakeSocketAddress; +import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; +import java.util.Arrays; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; +import org.mockito.stubbing.Answer; + +public class DeterministicSubsettingLoadBalancerTest { + + private List servers = Lists.newArrayList(); + private Map, Subchannel> subchannels = Maps.newLinkedHashMap(); + + private final Map subchannelStateListeners + = Maps.newLinkedHashMap(); + @Rule + public final MockitoRule mockitoRule = MockitoJUnit.rule(); + @Mock + private LoadBalancer.Helper mockHelper; + @Mock + private LoadBalancer mockChildLb; + @Mock + private SocketAddress mockSocketAddress; + + @Captor + private ArgumentCaptor resolvedAddrCaptor; + + private final LoadBalancerProvider mockChildLbProvider = new TestUtils.StandardLoadBalancerProvider( + "foo_policy") { + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return mockChildLb; + } + }; + + private DeterministicSubsettingLoadBalancer loadBalancer; + + private final LoadBalancerProvider roundRobinLbProvider = new TestUtils.StandardLoadBalancerProvider( + "round_robin") { + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return new RoundRobinLoadBalancer(helper); + } + }; + + private void setupBackends(int backendCount){ + servers = Lists.newArrayList(); + subchannels = Maps.newLinkedHashMap(); + for (int i = 0; i < backendCount; i++) { + SocketAddress addr = new FakeSocketAddress("server" + i); + EquivalentAddressGroup e = new EquivalentAddressGroup(addr); + servers.add(e); + Subchannel sc = mock(Subchannel.class); + subchannels.put(Arrays.asList(e), sc); + } + } + + @Before + public void setUp() { + loadBalancer = new DeterministicSubsettingLoadBalancer(mockHelper); + } + + public void addMock() { + when(mockHelper.createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class))).then( + new Answer() { + @Override + public Subchannel answer(InvocationOnMock invocation) throws Throwable { + LoadBalancer.CreateSubchannelArgs args = (LoadBalancer.CreateSubchannelArgs) invocation.getArguments()[0]; + final Subchannel subchannel = subchannels.get(args.getAddresses()); + when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); + when(subchannel.getAttributes()).thenReturn(args.getAttributes()); + doAnswer(new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + subchannelStateListeners.put(subchannel, + (LoadBalancer.SubchannelStateListener) invocation.getArguments()[0]); + return null; + } + }).when(subchannel).start(any(LoadBalancer.SubchannelStateListener.class)); + return subchannel; + } + }); + } + + @Test + public void acceptResolvedAddresses_mocked() { + int subsetSize = 3; + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(true) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)).build(); + + + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config).build(); + + + assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); + + verify(mockChildLb).handleResolvedAddresses( + resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build() + ); + } + + @Test + public void acceptResolvedAddresses() { + addMock(); + setupBackends(6); + int subsetSize = 3; + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); + + + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config).build(); + + assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); + + int insubset = 0; + for (Subchannel subchannel : subchannels.values()){ + LoadBalancer.SubchannelStateListener sc = subchannelStateListeners.get(subchannel); + if (sc != null) { // it might be null if it's not in the subset. + insubset += 1; + sc.onSubchannelState(ConnectivityStateInfo.forNonError(ConnectivityState.READY)); + } + } + + assertThat(insubset).isEqualTo(subsetSize); + } + + @Test + public void closesUnusedConns() { + addMock(); + setupBackends(6); + List configs = Lists.newArrayList(); + for (int i = 0; i < 2; i++) { + configs.add( + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(3) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build()); + } + Iterator scIterator = subchannels.values().iterator(); + scIterator.next(); //subchannel0 + Subchannel subchannel1 = scIterator.next(); + Subchannel subchannel2 = scIterator.next(); + scIterator.next(); //subchannel3 + Subchannel subchannel4 = scIterator.next(); + scIterator.next(); //subchannel5 + + // In the first call to RR.acceptResolvedAddresses, all subchannels will be new + // with nothing to close. in the second iteration, we need to remove the subchannels + // from the first subset. + List> subsets = Lists.newArrayList( + Lists.newArrayList(), + Lists.newArrayList(subchannel4, subchannel1, subchannel2)); + int newconns = 0; + + for (int i = 0; i < 2; i++) { + DeterministicSubsettingLoadBalancerConfig config = configs.get(i); + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config).build(); + loadBalancer.acceptResolvedAddresses(resolvedAddresses); + for (Subchannel sc : subsets.get(i)){ + verify(sc).shutdown(); + } + for (Subchannel sc : subchannels.values()){ + LoadBalancer.SubchannelStateListener ssl = subchannelStateListeners.get(sc); + if (ssl != null) { + newconns += 1; + ssl.onSubchannelState(ConnectivityStateInfo.forNonError(ConnectivityState.READY)); + } + } + subchannelStateListeners.clear(); + } + assertThat(newconns).isEqualTo(6); + } + + @Test + public void reusesConns() { + addMock(); + setupBackends(3); + List configs = Lists.newArrayList(); + for (int i = 0; i < 3; i++) { + configs.add( + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(3) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build()); + } + + List perRun = Lists.newArrayList(); + + for (DeterministicSubsettingLoadBalancerConfig config : configs) { + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config).build(); + loadBalancer.acceptResolvedAddresses(resolvedAddresses); + int numSubchannelsOpened = 0; + for (Subchannel subchannel : subchannels.values()) { + LoadBalancer.SubchannelStateListener sc = subchannelStateListeners.get(subchannel); + if (sc != null) { + sc.onSubchannelState(ConnectivityStateInfo.forNonError(ConnectivityState.READY)); + numSubchannelsOpened += 1; + } + } + perRun.add(numSubchannelsOpened); + subchannelStateListeners.clear(); + } + assertThat(perRun).isEqualTo(Lists.newArrayList(3,0,0)); + } + + + @Test + public void backendsCanBeDistributedEvenly() { + verifyCreatesSubsets(12, 8, 3, 0); + } + + @Test + public void backendsCanNotBeDistributedEvenly() { + verifyCreatesSubsets(37, 22, 5, 2); + } + + @Test + public void notEnoughClientsForLastRoung() { + verifyCreatesSubsets(20, 7, 5, 1); + } + + @Test + public void excludedBackendsInEveryRound() { + verifyCreatesSubsets(21, 8, 5, 1); + } + + + public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int maxDiff) { + setupBackends(backends); + List configs = Lists.newArrayList(); + for ( int i = 0; i < clients; i++ ) { + configs.add( + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)).build()); + } + + Map subsetDistn = Maps.newLinkedHashMap(); + + for (DeterministicSubsettingLoadBalancerConfig config : configs) { + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config).build(); + loadBalancer.acceptResolvedAddresses(resolvedAddresses); + verify(mockChildLb, atLeastOnce()).handleResolvedAddresses(resolvedAddrCaptor.capture()); + // Verify ChildLB is only getting subsetSize ResolvedAddresses each time + assertThat(resolvedAddrCaptor.getValue().getAddresses().size()).isEqualTo(subsetSize); + for (EquivalentAddressGroup eag: resolvedAddrCaptor.getValue().getAddresses()) { + for (SocketAddress addr : eag.getAddresses()) { + Integer prev = subsetDistn.getOrDefault(addr, 0); + subsetDistn.put(addr, prev + 1); + } + } + } + int conns = clients / (backends / subsetSize); + for(int subchannelConnections : subsetDistn.values()){ + // Algorithm guarantees as close to a balance in connection count as it can. + assertThat(subchannelConnections).isAtLeast(conns - maxDiff); + assertThat(subchannelConnections).isAtMost(conns + maxDiff); + } + } } diff --git a/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java b/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java index 18f9bbf549f..da704643451 100644 --- a/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/OutlierDetectionLoadBalancerTest.java @@ -1037,7 +1037,7 @@ public void mathChecksOut() { assertThat(stdev).isEqualTo(147.32277488562318); } - private static class FakeSocketAddress extends SocketAddress { + static class FakeSocketAddress extends SocketAddress { final String name; From 69627896a67050ece9b79bfc6dfcc198601f14e6 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Mon, 14 Aug 2023 17:56:11 -0400 Subject: [PATCH 07/25] small fixes --- .../grpc/util/DeterministicSubsettingLoadBalancer.java | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 37e2ce580c6..35771ec2614 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -34,12 +34,6 @@ public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses){ DeterministicSubsettingLoadBalancerConfig config = (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); - // The map should only retain entries for addresses in this latest update. - ArrayList allAddresses = new ArrayList<>(); - for (EquivalentAddressGroup addressGroup : resolvedAddresses.getAddresses()){ - allAddresses.addAll(addressGroup.getAddresses()); - } - switchLb.switchTo(config.childPolicy.getProvider()); ResolvedAddresses subsetAddresses = buildSubsets(resolvedAddresses, config); @@ -60,7 +54,7 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini if (addresses.size() <= config.subsetSize) return allAddresses; if (config.sortAddresses) { - // If we sort, we do so via destination hashcode. This is deterministic but differs from the goland instrumentation. + // If we sort, we do so via destination hashcode. This is deterministic but differs from the golang instrumentation. addresses.sort(new AddressComparator()); } @@ -68,7 +62,7 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini Integer subsetCount = backendCount / config.subsetSize; Integer round = config.clientIndex / subsetCount; - + Integer excludedCount = backendCount % config.subsetSize; Integer excludedStart = (round * excludedCount) % backendCount; Integer excludedEnd = (excludedStart + excludedCount) % backendCount; From 944d80fbc5a4ea288cafa0ef9522c894b88c5066 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Mon, 14 Aug 2023 18:16:53 -0400 Subject: [PATCH 08/25] remove DeterministicSubsettingSubchannel --- .../DeterministicSubsettingLoadBalancer.java | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 35771ec2614..6366b8867c8 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -131,8 +131,7 @@ protected Helper delegate() { @Override public Subchannel createSubchannel(CreateSubchannelArgs args) { - DeterministicSubsettingSubchannel subchannel = new DeterministicSubsettingSubchannel(delegate.createSubchannel(args)); - return subchannel; + return delegate.createSubchannel(args); } } @@ -144,20 +143,6 @@ public int compare(SocketAddress o1, SocketAddress o2){ } - class DeterministicSubsettingSubchannel extends ForwardingSubchannel { - - private final Subchannel delegate; - - DeterministicSubsettingSubchannel(Subchannel delegate) { - this.delegate = delegate; - } - - @Override - protected Subchannel delegate() { - return this.delegate; - } - } - public static final class DeterministicSubsettingLoadBalancerConfig { public final Integer clientIndex; From ac6bddff005f41d2269d0a5ffa276b1a664b3605 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Mon, 14 Aug 2023 18:19:09 -0400 Subject: [PATCH 09/25] remove ChildHelper --- .../DeterministicSubsettingLoadBalancer.java | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 6366b8867c8..04489c385b3 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -113,26 +113,7 @@ public void shutdown() { } public DeterministicSubsettingLoadBalancer(Helper helper){ - ChildHelper childHelper = new ChildHelper(checkNotNull(helper, "helper")); - switchLb = new GracefulSwitchLoadBalancer(childHelper); - } - - class ChildHelper extends ForwardingLoadBalancerHelper { - private Helper delegate; - - ChildHelper(Helper delegate){ - this.delegate = delegate; - } - - @Override - protected Helper delegate() { - return delegate; - } - - @Override - public Subchannel createSubchannel(CreateSubchannelArgs args) { - return delegate.createSubchannel(args); - } + switchLb = new GracefulSwitchLoadBalancer(checkNotNull(helper, "helper")); } class AddressComparator implements Comparator { From b7869c49fe07d5f6b18dd0cf2e6bc0f70f936569 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 15:31:36 -0400 Subject: [PATCH 10/25] comments, maxDiff --- ...terministicSubsettingLoadBalancerTest.java | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index e75760fe631..61ad4599276 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -25,6 +25,7 @@ import io.grpc.util.OutlierDetectionLoadBalancerTest.FakeSocketAddress; import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; import java.util.Arrays; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -252,21 +253,29 @@ public void reusesConns() { @Test public void backendsCanBeDistributedEvenly() { + // Backends can be distributed evenly, so they should be. Therefore, maxDiff = 0 verifyCreatesSubsets(12, 8, 3, 0); } @Test public void backendsCanNotBeDistributedEvenly() { + // Backends can't be distributed evenly because there are excluded backends in every round and + // not enough clients to fill the last round. This provides 2 opportunities for an backend to be + // excluded, so the maxDiff is its maximum, 2 verifyCreatesSubsets(37, 22, 5, 2); } @Test - public void notEnoughClientsForLastRoung() { + public void notEnoughClientsForLastRound() { + // There are no excluded backends in each round, but there are not enough clients for the last round, + // meaning there is only one chance for a backend to be excluded. Therefore, maxDiff =1 verifyCreatesSubsets(20, 7, 5, 1); } @Test public void excludedBackendsInEveryRound() { + // There are enough clients to fill the last round, but there are excluded backends in every round, + // meaning there is only one chance for a backend to be excluded. Therefore, maxDiff =1 verifyCreatesSubsets(21, 8, 5, 1); } @@ -299,11 +308,9 @@ public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int } } } - int conns = clients / (backends / subsetSize); - for(int subchannelConnections : subsetDistn.values()){ - // Algorithm guarantees as close to a balance in connection count as it can. - assertThat(subchannelConnections).isAtLeast(conns - maxDiff); - assertThat(subchannelConnections).isAtMost(conns + maxDiff); - } + int maxConns = Collections.max(subsetDistn.values()); + int minConns = Collections.min(subsetDistn.values()); + + assertThat(maxConns < minConns+maxConns).isTrue(); } } From ed96951058525c7a797c9ef67f5161044ccda6e7 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:03:30 -0400 Subject: [PATCH 11/25] one eag per addy --- .../util/DeterministicSubsettingLoadBalancer.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 04489c385b3..412522fe2f4 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -86,16 +86,9 @@ private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, Determini ArrayList eaglist = new ArrayList<>(); - for (EquivalentAddressGroup eag : allAddresses.getAddresses()) { - List addrs = Lists.newArrayList(); - for (SocketAddress addr :eag.getAddresses()) { - if (subset.contains(addr)) { - addrs.add(addr); - } - } - if (addrs.size() != 0) { - eaglist.add(new EquivalentAddressGroup(addrs)); - } + // Create new EAGs per address + for (SocketAddress addr : subset) { + eaglist.add(new EquivalentAddressGroup(addr)); } ResolvedAddresses.Builder builder = allAddresses.toBuilder(); From 3f9a619b9ce5d7890d214ec57c042bfce30950dd Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:13:49 -0400 Subject: [PATCH 12/25] static --- .../java/io/grpc/util/DeterministicSubsettingLoadBalancer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 412522fe2f4..d6ad906467d 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -109,7 +109,7 @@ public DeterministicSubsettingLoadBalancer(Helper helper){ switchLb = new GracefulSwitchLoadBalancer(checkNotNull(helper, "helper")); } - class AddressComparator implements Comparator { + private static class AddressComparator implements Comparator { @Override public int compare(SocketAddress o1, SocketAddress o2){ return o1.hashCode() - o2.hashCode(); From 9260f507136118357636625fd6e684863d6542fc Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:21:43 -0400 Subject: [PATCH 13/25] more errors --- .../util/DeterministicSubsettingLoadBalancerProvider.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java index a22be3cecc2..f96cddd0406 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java @@ -11,7 +11,6 @@ import io.grpc.internal.ServiceConfigUtil.PolicySelection; import java.util.List; import java.util.Map; -import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder; @Internal public final class DeterministicSubsettingLoadBalancerProvider extends LoadBalancerProvider { @@ -64,7 +63,8 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawC ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, LoadBalancerRegistry.getDefaultRegistry()); - Builder configBuilder = new Builder(); + DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder configBuilder = + new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder(); configBuilder.setChildPolicy((PolicySelection) selectedConfig.getConfig()); From ab4ff7aa25c648ea2b96182d60845336e32f6646 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:31:55 -0400 Subject: [PATCH 14/25] checkstyle --- api/src/test/java/io/grpc/LoadBalancerRegistryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java index f44206495ad..8cd04522a59 100644 --- a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java @@ -59,7 +59,7 @@ public void stockProviders() { assertThat(outlierDetection.getPriority()).isEqualTo(5); LoadBalancerProvider deterministicSubsetting = defaultRegistry.getProvider( - "deterministic_subsetting"); + "deterministic_subsetting"); assertThat(deterministicSubsetting.getClass().getName()).isEqualTo( "io.grpc.util.DeterministicSubsettingLoadBalancerProvider"); assertThat(deterministicSubsetting.getPriority()).isEqualTo(5); From 8e8aa9bc1e197b47b5d05a6dc7984b3a56aaaefa Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:33:55 -0400 Subject: [PATCH 15/25] fix test --- api/src/test/java/io/grpc/LoadBalancerRegistryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java index 8cd04522a59..0a10ea8b0fe 100644 --- a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java @@ -41,7 +41,7 @@ public void getClassesViaHardcoded_classesPresent() throws Exception { @Test public void stockProviders() { LoadBalancerRegistry defaultRegistry = LoadBalancerRegistry.getDefaultRegistry(); - assertThat(defaultRegistry.providers()).hasSize(4); + assertThat(defaultRegistry.providers()).hasSize(5); LoadBalancerProvider pickFirst = defaultRegistry.getProvider("pick_first"); assertThat(pickFirst).isInstanceOf(PickFirstLoadBalancerProvider.class); From 5a9e1692c95a12b12306a643d0156c0e5eb4775a Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Tue, 15 Aug 2023 16:43:58 -0400 Subject: [PATCH 16/25] checkstyle --- api/src/test/java/io/grpc/LoadBalancerRegistryTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java index 0a10ea8b0fe..26cfc38f2c0 100644 --- a/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java +++ b/api/src/test/java/io/grpc/LoadBalancerRegistryTest.java @@ -61,7 +61,7 @@ public void stockProviders() { LoadBalancerProvider deterministicSubsetting = defaultRegistry.getProvider( "deterministic_subsetting"); assertThat(deterministicSubsetting.getClass().getName()).isEqualTo( - "io.grpc.util.DeterministicSubsettingLoadBalancerProvider"); + "io.grpc.util.DeterministicSubsettingLoadBalancerProvider"); assertThat(deterministicSubsetting.getPriority()).isEqualTo(5); LoadBalancerProvider grpclb = defaultRegistry.getProvider("grpclb"); From 175177637203594e60e406085572a5cd87a9afd9 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 16 Aug 2023 13:26:29 -0400 Subject: [PATCH 17/25] checkstyle --- .../DeterministicSubsettingLoadBalancer.java | 51 ++++++++++----- ...inisticSubsettingLoadBalancerProvider.java | 25 ++++++-- ...terministicSubsettingLoadBalancerTest.java | 63 +++++++++++-------- 3 files changed, 92 insertions(+), 47 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index d6ad906467d..d5203756730 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -1,10 +1,25 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package io.grpc.util; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; -import com.google.common.collect.Lists; import io.grpc.EquivalentAddressGroup; import io.grpc.Internal; import io.grpc.LoadBalancer; @@ -21,7 +36,7 @@ * Wraps a child {@code LoadBalancer}, separating the total set of backends * into smaller subsets for the child balancer to balance across. * - * This implements deterministic subsetting gRFC: + *

This implements deterministic subsetting gRFC: * https://github.com/grpc/proposal/blob/master/A68-deterministic-subsetting-lb-policy.md */ @Internal @@ -30,31 +45,33 @@ public final class DeterministicSubsettingLoadBalancer extends LoadBalancer { private final GracefulSwitchLoadBalancer switchLb; @Override - public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses){ - DeterministicSubsettingLoadBalancerConfig config - = (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); + public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { + DeterministicSubsettingLoadBalancerConfig config = + (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); switchLb.switchTo(config.childPolicy.getProvider()); ResolvedAddresses subsetAddresses = buildSubsets(resolvedAddresses, config); switchLb.handleResolvedAddresses( - subsetAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()) - .build()); + subsetAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build()); return true; } // implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/383 - private ResolvedAddresses buildSubsets(ResolvedAddresses allAddresses, DeterministicSubsettingLoadBalancerConfig config){ + private ResolvedAddresses buildSubsets( + ResolvedAddresses allAddresses, + DeterministicSubsettingLoadBalancerConfig config) { // The map should only retain entries for addresses in this latest update. ArrayList addresses = new ArrayList<>(); - for (EquivalentAddressGroup addressGroup : allAddresses.getAddresses()){ + for (EquivalentAddressGroup addressGroup : allAddresses.getAddresses()) { addresses.addAll(addressGroup.getAddresses()); } if (addresses.size() <= config.subsetSize) return allAddresses; if (config.sortAddresses) { - // If we sort, we do so via destination hashcode. This is deterministic but differs from the golang instrumentation. + // If we sort, we do so via destination hashcode. + // This is deterministic but differs from the golang instrumentation. addresses.sort(new AddressComparator()); } @@ -105,13 +122,13 @@ public void shutdown() { switchLb.shutdown(); } - public DeterministicSubsettingLoadBalancer(Helper helper){ + public DeterministicSubsettingLoadBalancer(Helper helper) { switchLb = new GracefulSwitchLoadBalancer(checkNotNull(helper, "helper")); } private static class AddressComparator implements Comparator { @Override - public int compare(SocketAddress o1, SocketAddress o2){ + public int compare(SocketAddress o1, SocketAddress o2) { return o1.hashCode() - o2.hashCode(); } @@ -144,7 +161,7 @@ public static class Builder { Boolean sortAddresses; PolicySelection childPolicy; - public Builder setClientIndex (Integer clientIndex){ + public Builder setClientIndex(Integer clientIndex) { checkState(clientIndex != null); // Indices must be positive integers. checkState(clientIndex >= 0); @@ -152,7 +169,7 @@ public Builder setClientIndex (Integer clientIndex){ return this; } - public Builder setSubsetSize (Integer subsetSize){ + public Builder setSubsetSize(Integer subsetSize) { checkArgument(subsetSize != null); // subsetSize of 1 is equivalent to `pick_first`. Use that policy if that behavior is desired. // Fallback to default of 10 of condition is not satisfied. @@ -161,19 +178,19 @@ public Builder setSubsetSize (Integer subsetSize){ return this; } - public Builder setSortAddresses (Boolean sortAddresses){ + public Builder setSortAddresses(Boolean sortAddresses) { checkArgument(sortAddresses != null); this.sortAddresses = sortAddresses; return this; } - public Builder setChildPolicy (PolicySelection childPolicy){ + public Builder setChildPolicy(PolicySelection childPolicy) { checkState(childPolicy != null); this.childPolicy = childPolicy; return this; } - public DeterministicSubsettingLoadBalancerConfig build () { + public DeterministicSubsettingLoadBalancerConfig build() { checkState(childPolicy != null); checkState(clientIndex != null); return new DeterministicSubsettingLoadBalancerConfig(clientIndex, subsetSize, sortAddresses, childPolicy); diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java index f96cddd0406..a718723671c 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java @@ -1,3 +1,19 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package io.grpc.util; import io.grpc.Internal; @@ -51,8 +67,8 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawC Integer subsetSize = JsonUtil.getNumberAsInteger(rawConfig, "subsetSize"); Boolean sortAddresses = JsonUtil.getBoolean(rawConfig, "sortAddresses"); - List childConfigCandidates = ServiceConfigUtil.unwrapLoadBalancingConfigList( - JsonUtil.getListOfObjects(rawConfig, "childPolicy")); + List childConfigCandidates = ServiceConfigUtil + .unwrapLoadBalancingConfigList(JsonUtil.getListOfObjects(rawConfig, "childPolicy")); if (childConfigCandidates == null || childConfigCandidates.isEmpty()) { return ConfigOrError.fromError(Status.INTERNAL.withDescription( "No child policy in deterministic_subsetting LB policy " + rawConfig @@ -60,10 +76,11 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawC } ConfigOrError selectedConfig = - ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, + ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, LoadBalancerRegistry.getDefaultRegistry()); - DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder configBuilder = + DeterministicSubsettingLoadBalancer + .DeterministicSubsettingLoadBalancerConfig.Builder configBuilder = new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder(); configBuilder.setChildPolicy((PolicySelection) selectedConfig.getConfig()); diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index 61ad4599276..6cbfeede56e 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -16,21 +16,20 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; -import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.ResolvedAddresses; +import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancerProvider; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TestUtils; -import java.net.SocketAddress; -import io.grpc.util.OutlierDetectionLoadBalancerTest.FakeSocketAddress; import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; +import io.grpc.util.OutlierDetectionLoadBalancerTest.FakeSocketAddress; +import java.net.SocketAddress; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.mockito.ArgumentCaptor; @@ -47,7 +46,7 @@ public class DeterministicSubsettingLoadBalancerTest { private Map, Subchannel> subchannels = Maps.newLinkedHashMap(); private final Map subchannelStateListeners - = Maps.newLinkedHashMap(); + = Maps.newLinkedHashMap(); @Rule public final MockitoRule mockitoRule = MockitoJUnit.rule(); @Mock @@ -60,8 +59,8 @@ public class DeterministicSubsettingLoadBalancerTest { @Captor private ArgumentCaptor resolvedAddrCaptor; - private final LoadBalancerProvider mockChildLbProvider = new TestUtils.StandardLoadBalancerProvider( - "foo_policy") { + private final LoadBalancerProvider mockChildLbProvider = new TestUtils + .StandardLoadBalancerProvider("foo_policy") { @Override public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { return mockChildLb; @@ -70,8 +69,8 @@ public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { private DeterministicSubsettingLoadBalancer loadBalancer; - private final LoadBalancerProvider roundRobinLbProvider = new TestUtils.StandardLoadBalancerProvider( - "round_robin") { + private final LoadBalancerProvider roundRobinLbProvider = new TestUtils + .StandardLoadBalancerProvider("round_robin") { @Override public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { return new RoundRobinLoadBalancer(helper); @@ -100,7 +99,8 @@ public void addMock() { new Answer() { @Override public Subchannel answer(InvocationOnMock invocation) throws Throwable { - LoadBalancer.CreateSubchannelArgs args = (LoadBalancer.CreateSubchannelArgs) invocation.getArguments()[0]; + LoadBalancer.CreateSubchannelArgs args = (LoadBalancer.CreateSubchannelArgs) + invocation.getArguments()[0]; final Subchannel subchannel = subchannels.get(args.getAddresses()); when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); when(subchannel.getAttributes()).thenReturn(args.getAttributes()); @@ -120,21 +120,25 @@ public Void answer(InvocationOnMock invocation) throws Throwable { @Test public void acceptResolvedAddresses_mocked() { int subsetSize = 3; - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig.Builder() + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig + .Builder() .setSubsetSize(subsetSize) .setClientIndex(0) .setSortAddresses(true) .setChildPolicy(new PolicySelection(mockChildLbProvider, null)).build(); - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses( + ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) .setLoadBalancingPolicyConfig(config).build(); assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); verify(mockChildLb).handleResolvedAddresses( - resolvedAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build() + resolvedAddresses + .toBuilder() + .setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build() ); } @@ -143,20 +147,22 @@ public void acceptResolvedAddresses() { addMock(); setupBackends(6); int subsetSize = 3; - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig.Builder() + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig + .Builder() .setSubsetSize(subsetSize) .setClientIndex(0) .setSortAddresses(false) .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) .setLoadBalancingPolicyConfig(config).build(); assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); int insubset = 0; - for (Subchannel subchannel : subchannels.values()){ + for (Subchannel subchannel : subchannels.values()) { LoadBalancer.SubchannelStateListener sc = subchannelStateListeners.get(subchannel); if (sc != null) { // it might be null if it's not in the subset. insubset += 1; @@ -198,13 +204,14 @@ public void closesUnusedConns() { for (int i = 0; i < 2; i++) { DeterministicSubsettingLoadBalancerConfig config = configs.get(i); - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) .setLoadBalancingPolicyConfig(config).build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); - for (Subchannel sc : subsets.get(i)){ + for (Subchannel sc : subsets.get(i)) { verify(sc).shutdown(); } - for (Subchannel sc : subchannels.values()){ + for (Subchannel sc : subchannels.values()) { LoadBalancer.SubchannelStateListener ssl = subchannelStateListeners.get(sc); if (ssl != null) { newconns += 1; @@ -233,7 +240,8 @@ public void reusesConns() { List perRun = Lists.newArrayList(); for (DeterministicSubsettingLoadBalancerConfig config : configs) { - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) .setLoadBalancingPolicyConfig(config).build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); int numSubchannelsOpened = 0; @@ -260,22 +268,24 @@ public void backendsCanBeDistributedEvenly() { @Test public void backendsCanNotBeDistributedEvenly() { // Backends can't be distributed evenly because there are excluded backends in every round and - // not enough clients to fill the last round. This provides 2 opportunities for an backend to be + // not enough clients to fill the last round. This provides 2 opportunities for a backend to be // excluded, so the maxDiff is its maximum, 2 verifyCreatesSubsets(37, 22, 5, 2); } @Test public void notEnoughClientsForLastRound() { - // There are no excluded backends in each round, but there are not enough clients for the last round, - // meaning there is only one chance for a backend to be excluded. Therefore, maxDiff =1 + // There are no excluded backends in each round, but there are not enough clients for the + // last round, meaning there is only one chance for a backend to be excluded. + // Therefore, maxDiff =1 verifyCreatesSubsets(20, 7, 5, 1); } @Test public void excludedBackendsInEveryRound() { - // There are enough clients to fill the last round, but there are excluded backends in every round, - // meaning there is only one chance for a backend to be excluded. Therefore, maxDiff =1 + // There are enough clients to fill the last round, but there are excluded backends + // in every round, meaning there is only one chance for a backend to be excluded. + // Therefore, maxDiff =1 verifyCreatesSubsets(21, 8, 5, 1); } @@ -295,7 +305,8 @@ public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int Map subsetDistn = Maps.newLinkedHashMap(); for (DeterministicSubsettingLoadBalancerConfig config : configs) { - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses(ImmutableList.copyOf(servers)) + ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) .setLoadBalancingPolicyConfig(config).build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); verify(mockChildLb, atLeastOnce()).handleResolvedAddresses(resolvedAddrCaptor.capture()); From e31cd3dfe853e93fa0fb58d799dbad004965ed7d Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 16 Aug 2023 13:40:58 -0400 Subject: [PATCH 18/25] use toString --- .../util/DeterministicSubsettingLoadBalancer.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index d5203756730..46c6767b436 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -70,8 +70,7 @@ private ResolvedAddresses buildSubsets( if (addresses.size() <= config.subsetSize) return allAddresses; if (config.sortAddresses) { - // If we sort, we do so via destination hashcode. - // This is deterministic but differs from the golang instrumentation. + // If we sort, we do so via the string representation of the SocketAddress. addresses.sort(new AddressComparator()); } @@ -127,9 +126,13 @@ public DeterministicSubsettingLoadBalancer(Helper helper) { } private static class AddressComparator implements Comparator { + // For consistency with the golang instrumentation, this assumes toString is overridden such + // that it is a string representation of an IP. Though any string representation of a + // SocketAddress will work here, other definitions of toString may yield differing results from + // the golang instrumentation. @Override public int compare(SocketAddress o1, SocketAddress o2) { - return o1.hashCode() - o2.hashCode(); + return o1.toString().compareTo(o2.toString()); } } @@ -193,7 +196,11 @@ public Builder setChildPolicy(PolicySelection childPolicy) { public DeterministicSubsettingLoadBalancerConfig build() { checkState(childPolicy != null); checkState(clientIndex != null); - return new DeterministicSubsettingLoadBalancerConfig(clientIndex, subsetSize, sortAddresses, childPolicy); + return new DeterministicSubsettingLoadBalancerConfig( + clientIndex, + subsetSize, + sortAddresses, + childPolicy); } } } From 182e5f7bcfd3928100324a63f2f5ee0030017568 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 16 Aug 2023 13:44:06 -0400 Subject: [PATCH 19/25] use plugin --- .../DeterministicSubsettingLoadBalancer.java | 31 ++- ...inisticSubsettingLoadBalancerProvider.java | 33 +-- ...ticSubsettingLoadBalancerProviderTest.java | 30 ++- ...terministicSubsettingLoadBalancerTest.java | 244 +++++++++--------- 4 files changed, 177 insertions(+), 161 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index 46c6767b436..fc0ce0d9288 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -33,10 +33,10 @@ import java.util.Random; /** - * Wraps a child {@code LoadBalancer}, separating the total set of backends - * into smaller subsets for the child balancer to balance across. + * Wraps a child {@code LoadBalancer}, separating the total set of backends into smaller subsets for + * the child balancer to balance across. * - *

This implements deterministic subsetting gRFC: + *

This implements deterministic subsetting gRFC: * https://github.com/grpc/proposal/blob/master/A68-deterministic-subsetting-lb-policy.md */ @Internal @@ -47,21 +47,24 @@ public final class DeterministicSubsettingLoadBalancer extends LoadBalancer { @Override public boolean acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { DeterministicSubsettingLoadBalancerConfig config = - (DeterministicSubsettingLoadBalancerConfig) resolvedAddresses.getLoadBalancingPolicyConfig(); + (DeterministicSubsettingLoadBalancerConfig) + resolvedAddresses.getLoadBalancingPolicyConfig(); switchLb.switchTo(config.childPolicy.getProvider()); ResolvedAddresses subsetAddresses = buildSubsets(resolvedAddresses, config); switchLb.handleResolvedAddresses( - subsetAddresses.toBuilder().setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build()); + subsetAddresses.toBuilder() + .setLoadBalancingPolicyConfig(config.childPolicy.getConfig()) + .build()); return true; } - // implements the subsetting algorithm, as described in A68: https://github.com/grpc/proposal/pull/383 + // implements the subsetting algorithm, as described in A68: + // https://github.com/grpc/proposal/pull/383 private ResolvedAddresses buildSubsets( - ResolvedAddresses allAddresses, - DeterministicSubsettingLoadBalancerConfig config) { + ResolvedAddresses allAddresses, DeterministicSubsettingLoadBalancerConfig config) { // The map should only retain entries for addresses in this latest update. ArrayList addresses = new ArrayList<>(); for (EquivalentAddressGroup addressGroup : allAddresses.getAddresses()) { @@ -100,7 +103,7 @@ private ResolvedAddresses buildSubsets( List subset = addresses.subList(start, end); - ArrayList eaglist = new ArrayList<>(); + ArrayList eaglist = new ArrayList<>(); // Create new EAGs per address for (SocketAddress addr : subset) { @@ -134,7 +137,6 @@ private static class AddressComparator implements Comparator { public int compare(SocketAddress o1, SocketAddress o2) { return o1.toString().compareTo(o2.toString()); } - } public static final class DeterministicSubsettingLoadBalancerConfig { @@ -156,7 +158,6 @@ private DeterministicSubsettingLoadBalancerConfig( this.childPolicy = childPolicy; } - public static class Builder { Integer clientIndex; Integer subsetSize = 10; @@ -174,7 +175,8 @@ public Builder setClientIndex(Integer clientIndex) { public Builder setSubsetSize(Integer subsetSize) { checkArgument(subsetSize != null); - // subsetSize of 1 is equivalent to `pick_first`. Use that policy if that behavior is desired. + // subsetSize of 1 is equivalent to `pick_first`. Use that policy if that behavior is + // desired. // Fallback to default of 10 of condition is not satisfied. checkArgument(subsetSize > 1); this.subsetSize = subsetSize; @@ -197,10 +199,7 @@ public DeterministicSubsettingLoadBalancerConfig build() { checkState(childPolicy != null); checkState(clientIndex != null); return new DeterministicSubsettingLoadBalancerConfig( - clientIndex, - subsetSize, - sortAddresses, - childPolicy); + clientIndex, subsetSize, sortAddresses, childPolicy); } } } diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java index a718723671c..bc967564b0b 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancerProvider.java @@ -57,8 +57,9 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map rawConfig) { return parseLoadBalancingPolicyConfigInternal(rawConfig); } catch (RuntimeException e) { return ConfigOrError.fromError( - Status.UNAVAILABLE.withCause(e).withDescription( - "Failed parsing configuration for " + getPolicyName())); + Status.UNAVAILABLE + .withCause(e) + .withDescription("Failed parsing configuration for " + getPolicyName())); } } @@ -67,30 +68,32 @@ private ConfigOrError parseLoadBalancingPolicyConfigInternal(Map rawC Integer subsetSize = JsonUtil.getNumberAsInteger(rawConfig, "subsetSize"); Boolean sortAddresses = JsonUtil.getBoolean(rawConfig, "sortAddresses"); - List childConfigCandidates = ServiceConfigUtil - .unwrapLoadBalancingConfigList(JsonUtil.getListOfObjects(rawConfig, "childPolicy")); + List childConfigCandidates = + ServiceConfigUtil.unwrapLoadBalancingConfigList( + JsonUtil.getListOfObjects(rawConfig, "childPolicy")); if (childConfigCandidates == null || childConfigCandidates.isEmpty()) { - return ConfigOrError.fromError(Status.INTERNAL.withDescription( - "No child policy in deterministic_subsetting LB policy " + rawConfig - )); + return ConfigOrError.fromError( + Status.INTERNAL.withDescription( + "No child policy in deterministic_subsetting LB policy " + rawConfig)); } ConfigOrError selectedConfig = - ServiceConfigUtil.selectLbPolicyFromList(childConfigCandidates, - LoadBalancerRegistry.getDefaultRegistry()); + ServiceConfigUtil.selectLbPolicyFromList( + childConfigCandidates, LoadBalancerRegistry.getDefaultRegistry()); - DeterministicSubsettingLoadBalancer - .DeterministicSubsettingLoadBalancerConfig.Builder configBuilder = - new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder(); + DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder + configBuilder = + new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig + .Builder(); configBuilder.setChildPolicy((PolicySelection) selectedConfig.getConfig()); if (clientIndex != null) { configBuilder.setClientIndex(clientIndex); } else { - return ConfigOrError.fromError(Status.INTERNAL.withDescription( - "No client index set, cannot determine subsets " + rawConfig) - ); + return ConfigOrError.fromError( + Status.INTERNAL.withDescription( + "No client index set, cannot determine subsets " + rawConfig)); } if (subsetSize != null) { diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index b739b1a5432..f098947a9cd 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -20,11 +20,14 @@ @RunWith(JUnit4.class) public class DeterministicSubsettingLoadBalancerProviderTest { - private final DeterministicSubsettingLoadBalancerProvider provider = new DeterministicSubsettingLoadBalancerProvider(); + private final DeterministicSubsettingLoadBalancerProvider provider = + new DeterministicSubsettingLoadBalancerProvider(); + @Test public void registered() { - for (LoadBalancerProvider current : InternalServiceProviders.getCandidatesViaServiceLoader( - LoadBalancerProvider.class, getClass().getClassLoader())) { + for (LoadBalancerProvider current : + InternalServiceProviders.getCandidatesViaServiceLoader( + LoadBalancerProvider.class, getClass().getClassLoader())) { if (current instanceof DeterministicSubsettingLoadBalancerProvider) { return; } @@ -35,15 +38,14 @@ LoadBalancerProvider.class, getClass().getClassLoader())) { @Test public void providesLoadBalancer() { Helper helper = mock(Helper.class); - assertThat(provider.newLoadBalancer(helper)).isInstanceOf(DeterministicSubsettingLoadBalancer.class); + assertThat(provider.newLoadBalancer(helper)) + .isInstanceOf(DeterministicSubsettingLoadBalancer.class); } @Test public void parseConfigRequiresClientIdx() throws IOException { - String lbConfig = - "{ \"clientIndex\" : null }"; - String lbConfig2 = - "{ \"clientIndex\" : -1 }"; + String lbConfig = "{ \"clientIndex\" : null }"; + String lbConfig2 = "{ \"clientIndex\" : -1 }"; ArrayList configs = new ArrayList<>(); configs.add(lbConfig); configs.add(lbConfig2); @@ -58,13 +60,15 @@ public void parseConfigRequiresClientIdx() throws IOException { @Test public void parseConfigWithDefaults() throws IOException { String lbConfig = - "{ \"clientIndex\" : 0, " - + "\"childPolicy\" : [{\"round_robin\" : {}}], " - + "\"sortAddresses\" : false }"; - ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + "{ \"clientIndex\" : 0, " + + "\"childPolicy\" : [{\"round_robin\" : {}}], " + + "\"sortAddresses\" : false }"; + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); System.out.println(configOrError); assertThat(configOrError.getConfig()).isNotNull(); - DeterministicSubsettingLoadBalancerConfig config = (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); + DeterministicSubsettingLoadBalancerConfig config = + (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); assertThat(config.clientIndex).isEqualTo(0); assertThat(config.sortAddresses).isEqualTo(false); diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index 6cbfeede56e..f96a31c39bf 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -1,6 +1,5 @@ package io.grpc.util; - import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.atLeastOnce; @@ -45,39 +44,34 @@ public class DeterministicSubsettingLoadBalancerTest { private List servers = Lists.newArrayList(); private Map, Subchannel> subchannels = Maps.newLinkedHashMap(); - private final Map subchannelStateListeners - = Maps.newLinkedHashMap(); - @Rule - public final MockitoRule mockitoRule = MockitoJUnit.rule(); - @Mock - private LoadBalancer.Helper mockHelper; - @Mock - private LoadBalancer mockChildLb; - @Mock - private SocketAddress mockSocketAddress; - - @Captor - private ArgumentCaptor resolvedAddrCaptor; - - private final LoadBalancerProvider mockChildLbProvider = new TestUtils - .StandardLoadBalancerProvider("foo_policy") { - @Override - public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { - return mockChildLb; - } - }; + private final Map subchannelStateListeners = + Maps.newLinkedHashMap(); + @Rule public final MockitoRule mockitoRule = MockitoJUnit.rule(); + @Mock private LoadBalancer.Helper mockHelper; + @Mock private LoadBalancer mockChildLb; + @Mock private SocketAddress mockSocketAddress; + + @Captor private ArgumentCaptor resolvedAddrCaptor; + + private final LoadBalancerProvider mockChildLbProvider = + new TestUtils.StandardLoadBalancerProvider("foo_policy") { + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return mockChildLb; + } + }; private DeterministicSubsettingLoadBalancer loadBalancer; - private final LoadBalancerProvider roundRobinLbProvider = new TestUtils - .StandardLoadBalancerProvider("round_robin") { - @Override - public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { - return new RoundRobinLoadBalancer(helper); - } - }; + private final LoadBalancerProvider roundRobinLbProvider = + new TestUtils.StandardLoadBalancerProvider("round_robin") { + @Override + public LoadBalancer newLoadBalancer(LoadBalancer.Helper helper) { + return new RoundRobinLoadBalancer(helper); + } + }; - private void setupBackends(int backendCount){ + private void setupBackends(int backendCount) { servers = Lists.newArrayList(); subchannels = Maps.newLinkedHashMap(); for (int i = 0; i < backendCount; i++) { @@ -95,51 +89,58 @@ public void setUp() { } public void addMock() { - when(mockHelper.createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class))).then( - new Answer() { - @Override - public Subchannel answer(InvocationOnMock invocation) throws Throwable { - LoadBalancer.CreateSubchannelArgs args = (LoadBalancer.CreateSubchannelArgs) - invocation.getArguments()[0]; - final Subchannel subchannel = subchannels.get(args.getAddresses()); - when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); - when(subchannel.getAttributes()).thenReturn(args.getAttributes()); - doAnswer(new Answer() { - @Override - public Void answer(InvocationOnMock invocation) throws Throwable { - subchannelStateListeners.put(subchannel, - (LoadBalancer.SubchannelStateListener) invocation.getArguments()[0]); - return null; - } - }).when(subchannel).start(any(LoadBalancer.SubchannelStateListener.class)); - return subchannel; - } - }); + when(mockHelper.createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class))) + .then( + new Answer() { + @Override + public Subchannel answer(InvocationOnMock invocation) throws Throwable { + LoadBalancer.CreateSubchannelArgs args = + (LoadBalancer.CreateSubchannelArgs) invocation.getArguments()[0]; + final Subchannel subchannel = subchannels.get(args.getAddresses()); + when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); + when(subchannel.getAttributes()).thenReturn(args.getAttributes()); + doAnswer( + new Answer() { + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + subchannelStateListeners.put( + subchannel, + (LoadBalancer.SubchannelStateListener) + invocation.getArguments()[0]); + return null; + } + }) + .when(subchannel) + .start(any(LoadBalancer.SubchannelStateListener.class)); + return subchannel; + } + }); } @Test public void acceptResolvedAddresses_mocked() { int subsetSize = 3; - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig - .Builder() - .setSubsetSize(subsetSize) - .setClientIndex(0) - .setSortAddresses(true) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)).build(); - - - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder().setAddresses( - ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) - .setLoadBalancingPolicyConfig(config).build(); - + DeterministicSubsettingLoadBalancerConfig config = + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(true) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config) + .build(); assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); - verify(mockChildLb).handleResolvedAddresses( - resolvedAddresses - .toBuilder() - .setLoadBalancingPolicyConfig(config.childPolicy.getConfig()).build() - ); + verify(mockChildLb) + .handleResolvedAddresses( + resolvedAddresses.toBuilder() + .setLoadBalancingPolicyConfig(config.childPolicy.getConfig()) + .build()); } @Test @@ -147,17 +148,19 @@ public void acceptResolvedAddresses() { addMock(); setupBackends(6); int subsetSize = 3; - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancerConfig - .Builder() - .setSubsetSize(subsetSize) - .setClientIndex(0) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build(); - - - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.copyOf(servers)) - .setLoadBalancingPolicyConfig(config).build(); + DeterministicSubsettingLoadBalancerConfig config = + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)) + .build(); + + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config) + .build(); assertThat(loadBalancer.acceptResolvedAddresses(resolvedAddresses)).isTrue(); @@ -180,33 +183,36 @@ public void closesUnusedConns() { List configs = Lists.newArrayList(); for (int i = 0; i < 2; i++) { configs.add( - new DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(3) - .setClientIndex(i) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build()); + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(3) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)) + .build()); } Iterator scIterator = subchannels.values().iterator(); - scIterator.next(); //subchannel0 + scIterator.next(); // subchannel0 Subchannel subchannel1 = scIterator.next(); Subchannel subchannel2 = scIterator.next(); - scIterator.next(); //subchannel3 + scIterator.next(); // subchannel3 Subchannel subchannel4 = scIterator.next(); - scIterator.next(); //subchannel5 + scIterator.next(); // subchannel5 // In the first call to RR.acceptResolvedAddresses, all subchannels will be new // with nothing to close. in the second iteration, we need to remove the subchannels // from the first subset. - List> subsets = Lists.newArrayList( - Lists.newArrayList(), - Lists.newArrayList(subchannel4, subchannel1, subchannel2)); + List> subsets = + Lists.newArrayList( + Lists.newArrayList(), Lists.newArrayList(subchannel4, subchannel1, subchannel2)); int newconns = 0; for (int i = 0; i < 2; i++) { DeterministicSubsettingLoadBalancerConfig config = configs.get(i); - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.copyOf(servers)) - .setLoadBalancingPolicyConfig(config).build(); + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config) + .build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); for (Subchannel sc : subsets.get(i)) { verify(sc).shutdown(); @@ -230,19 +236,22 @@ public void reusesConns() { List configs = Lists.newArrayList(); for (int i = 0; i < 3; i++) { configs.add( - new DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(3) - .setClientIndex(i) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)).build()); + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(3) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(roundRobinLbProvider, null)) + .build()); } List perRun = Lists.newArrayList(); for (DeterministicSubsettingLoadBalancerConfig config : configs) { - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.copyOf(servers)) - .setLoadBalancingPolicyConfig(config).build(); + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config) + .build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); int numSubchannelsOpened = 0; for (Subchannel subchannel : subchannels.values()) { @@ -255,10 +264,9 @@ public void reusesConns() { perRun.add(numSubchannelsOpened); subchannelStateListeners.clear(); } - assertThat(perRun).isEqualTo(Lists.newArrayList(3,0,0)); + assertThat(perRun).isEqualTo(Lists.newArrayList(3, 0, 0)); } - @Test public void backendsCanBeDistributedEvenly() { // Backends can be distributed evenly, so they should be. Therefore, maxDiff = 0 @@ -289,39 +297,41 @@ public void excludedBackendsInEveryRound() { verifyCreatesSubsets(21, 8, 5, 1); } - public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int maxDiff) { setupBackends(backends); List configs = Lists.newArrayList(); - for ( int i = 0; i < clients; i++ ) { + for (int i = 0; i < clients; i++) { configs.add( - new DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(subsetSize) - .setClientIndex(i) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)).build()); + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(i) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build()); } Map subsetDistn = Maps.newLinkedHashMap(); for (DeterministicSubsettingLoadBalancerConfig config : configs) { - ResolvedAddresses resolvedAddresses = ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.copyOf(servers)) - .setLoadBalancingPolicyConfig(config).build(); + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(servers)) + .setLoadBalancingPolicyConfig(config) + .build(); loadBalancer.acceptResolvedAddresses(resolvedAddresses); verify(mockChildLb, atLeastOnce()).handleResolvedAddresses(resolvedAddrCaptor.capture()); // Verify ChildLB is only getting subsetSize ResolvedAddresses each time assertThat(resolvedAddrCaptor.getValue().getAddresses().size()).isEqualTo(subsetSize); - for (EquivalentAddressGroup eag: resolvedAddrCaptor.getValue().getAddresses()) { + for (EquivalentAddressGroup eag : resolvedAddrCaptor.getValue().getAddresses()) { for (SocketAddress addr : eag.getAddresses()) { - Integer prev = subsetDistn.getOrDefault(addr, 0); - subsetDistn.put(addr, prev + 1); - } + Integer prev = subsetDistn.getOrDefault(addr, 0); + subsetDistn.put(addr, prev + 1); } } + } int maxConns = Collections.max(subsetDistn.values()); int minConns = Collections.min(subsetDistn.values()); - assertThat(maxConns < minConns+maxConns).isTrue(); + assertThat(maxConns < minConns + maxConns).isTrue(); } } From 0bfd0748cf67e31624f30ba09bef7e7a570a3adb Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 16 Aug 2023 14:05:57 -0400 Subject: [PATCH 20/25] if {} --- .../io/grpc/util/DeterministicSubsettingLoadBalancer.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index fc0ce0d9288..dfbd8ac6b83 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -71,7 +71,9 @@ private ResolvedAddresses buildSubsets( addresses.addAll(addressGroup.getAddresses()); } - if (addresses.size() <= config.subsetSize) return allAddresses; + if (addresses.size() <= config.subsetSize) { + return allAddresses; + } if (config.sortAddresses) { // If we sort, we do so via the string representation of the SocketAddress. addresses.sort(new AddressComparator()); From 7fdcfc9134d763d28c55c380ca7eb305bf6b4749 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Wed, 16 Aug 2023 15:44:45 -0400 Subject: [PATCH 21/25] final fixes --- ...ticSubsettingLoadBalancerProviderTest.java | 16 +++++ ...terministicSubsettingLoadBalancerTest.java | 59 +++++++++++-------- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index f098947a9cd..b106303e634 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package io.grpc.util; import static com.google.common.truth.Truth.assertThat; diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index f96a31c39bf..99f238d02d3 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -1,3 +1,19 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + package io.grpc.util; import static com.google.common.truth.Truth.assertThat; @@ -15,8 +31,10 @@ import io.grpc.ConnectivityStateInfo; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; +import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.ResolvedAddresses; import io.grpc.LoadBalancer.Subchannel; +import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.LoadBalancerProvider; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TestUtils; @@ -89,32 +107,25 @@ public void setUp() { } public void addMock() { - when(mockHelper.createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class))) - .then( - new Answer() { + when(mockHelper.createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class))).then( + new Answer() { + @Override + public Subchannel answer(InvocationOnMock invocation) throws Throwable { + CreateSubchannelArgs args = (CreateSubchannelArgs) invocation.getArguments()[0]; + final Subchannel subchannel = subchannels.get(args.getAddresses()); + when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); + when(subchannel.getAttributes()).thenReturn(args.getAttributes()); + doAnswer(new Answer() { @Override - public Subchannel answer(InvocationOnMock invocation) throws Throwable { - LoadBalancer.CreateSubchannelArgs args = - (LoadBalancer.CreateSubchannelArgs) invocation.getArguments()[0]; - final Subchannel subchannel = subchannels.get(args.getAddresses()); - when(subchannel.getAllAddresses()).thenReturn(args.getAddresses()); - when(subchannel.getAttributes()).thenReturn(args.getAttributes()); - doAnswer( - new Answer() { - @Override - public Void answer(InvocationOnMock invocation) throws Throwable { - subchannelStateListeners.put( - subchannel, - (LoadBalancer.SubchannelStateListener) - invocation.getArguments()[0]); - return null; - } - }) - .when(subchannel) - .start(any(LoadBalancer.SubchannelStateListener.class)); - return subchannel; + public Void answer(InvocationOnMock invocation) throws Throwable { + subchannelStateListeners.put(subchannel, + (SubchannelStateListener) invocation.getArguments()[0]); + return null; } - }); + }).when(subchannel).start(any(SubchannelStateListener.class)); + return subchannel; + } + }); } @Test From 76d686b024efb5120ff07d4b9a0c35bb7d9a4c11 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Fri, 18 Aug 2023 14:31:01 -0400 Subject: [PATCH 22/25] cover uncovered lines with tests --- .../DeterministicSubsettingLoadBalancer.java | 4 +- ...ticSubsettingLoadBalancerProviderTest.java | 31 +++-- ...terministicSubsettingLoadBalancerTest.java | 124 +++++++++++++++++- 3 files changed, 147 insertions(+), 12 deletions(-) diff --git a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java index dfbd8ac6b83..f0301cef0e3 100644 --- a/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java +++ b/util/src/main/java/io/grpc/util/DeterministicSubsettingLoadBalancer.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import com.google.common.annotations.VisibleForTesting; import io.grpc.EquivalentAddressGroup; import io.grpc.Internal; import io.grpc.LoadBalancer; @@ -130,7 +131,8 @@ public DeterministicSubsettingLoadBalancer(Helper helper) { switchLb = new GracefulSwitchLoadBalancer(checkNotNull(helper, "helper")); } - private static class AddressComparator implements Comparator { + @VisibleForTesting + static class AddressComparator implements Comparator { // For consistency with the golang instrumentation, this assumes toString is overridden such // that it is a string representation of an IP. Though any string representation of a // SocketAddress will work here, other definitions of toString may yield differing results from diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index b106303e634..d2864c6b2bf 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -24,6 +24,7 @@ import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancerProvider; import io.grpc.NameResolver.ConfigOrError; +import io.grpc.Status; import io.grpc.internal.JsonParser; import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; import java.io.IOException; @@ -60,17 +61,12 @@ public void providesLoadBalancer() { @Test public void parseConfigRequiresClientIdx() throws IOException { - String lbConfig = "{ \"clientIndex\" : null }"; - String lbConfig2 = "{ \"clientIndex\" : -1 }"; - ArrayList configs = new ArrayList<>(); - configs.add(lbConfig); - configs.add(lbConfig2); + String config = "{ \"childPolicy\" : [{\"round_robin\" : {}}] } "; - ConfigOrError configOrError; - for (String config : configs) { - configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(config)); - assertThat(configOrError.getError()).isNotNull(); - } + ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(config)); + assertThat(configOrError.getError()).isNotNull(); + assertThat(configOrError.getError().toString()).isEqualTo(Status.INTERNAL.withDescription( + "No client index set, cannot determine subsets {childPolicy=[{round_robin={}}]}" ).toString()); } @Test @@ -97,4 +93,19 @@ public void parseConfigWithDefaults() throws IOException { private static Map parseJsonObject(String json) throws IOException { return (Map) JsonParser.parse(json); } + + @Test + public void parseConfigWithCustomSubsetSize() throws IOException { + String lbConfig = + "{ \"clientIndex\" : 0, " + + "\"subsetSize\" : 3, " + + "\"childPolicy\" : [{\"round_robin\" : {}}], " + + "\"sortAddresses\" : false }"; + + ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + assertThat(configOrError.getConfig()).isNotNull(); + DeterministicSubsettingLoadBalancerConfig config = + (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); + assertThat(config.subsetSize).isEqualTo(3); + } } diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index 99f238d02d3..8ced4dea8d6 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -36,6 +36,7 @@ import io.grpc.LoadBalancer.Subchannel; import io.grpc.LoadBalancer.SubchannelStateListener; import io.grpc.LoadBalancerProvider; +import io.grpc.Status; import io.grpc.internal.ServiceConfigUtil.PolicySelection; import io.grpc.internal.TestUtils; import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; @@ -46,6 +47,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Random; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -68,6 +70,10 @@ public class DeterministicSubsettingLoadBalancerTest { @Mock private LoadBalancer.Helper mockHelper; @Mock private LoadBalancer mockChildLb; @Mock private SocketAddress mockSocketAddress; + @Captor + private ArgumentCaptor connectivityStateCaptor; + @Captor + private ArgumentCaptor errorPickerCaptor; @Captor private ArgumentCaptor resolvedAddrCaptor; @@ -128,6 +134,69 @@ public Void answer(InvocationOnMock invocation) throws Throwable { }); } + @Test + public void handleNameResoutionError_noChildLb() { + loadBalancer.handleNameResolutionError(Status.DEADLINE_EXCEEDED); + + verify(mockHelper).updateBalancingState(connectivityStateCaptor.capture(), + errorPickerCaptor.capture()); + assertThat(connectivityStateCaptor.getValue()).isEqualTo(ConnectivityState.TRANSIENT_FAILURE); + } + @Test + public void handleNameResolutionError_withChildLb() { + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(2) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + loadBalancer.acceptResolvedAddresses(ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config) + .build()); + loadBalancer.handleNameResolutionError(Status.DEADLINE_EXCEEDED); + + verify(mockChildLb).handleNameResolutionError(Status.DEADLINE_EXCEEDED); + } + + @Test + public void shutdown() { + DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(2) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + loadBalancer.acceptResolvedAddresses(ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config) + .build()); + loadBalancer.shutdown(); + verify(mockChildLb).shutdown(); + } + + @Test + public void addressComparator() { + setupBackends(5); + List sorted = Lists.newArrayList(); + for (EquivalentAddressGroup eag: servers) { + sorted.addAll(eag.getAddresses()); + } + + Collections.shuffle(servers); + List addresses = Lists.newArrayList(); + for (EquivalentAddressGroup eag: servers) { + addresses.addAll(eag.getAddresses()); + } + + assertThat(addresses).isNotEqualTo(sorted); + addresses.sort(new DeterministicSubsettingLoadBalancer.AddressComparator()); + + assertThat(addresses).isEqualTo(sorted); + } + @Test public void acceptResolvedAddresses_mocked() { int subsetSize = 3; @@ -187,6 +256,52 @@ public void acceptResolvedAddresses() { assertThat(insubset).isEqualTo(subsetSize); } + @Test + public void sortingBackends() { + setupBackends(4); + // Shuffle servers so that they're not in 0, 1, 2 order + List shuffledServers = Lists.newArrayList( + servers.get(1), servers.get(3), servers.get(2), servers.get(0)); + int subsetSize = 2; + DeterministicSubsettingLoadBalancerConfig sortConfig = + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(true) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + DeterministicSubsettingLoadBalancerConfig dontSortConfig = + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + List configs = Lists.newArrayList(sortConfig, dontSortConfig); + List> actual = Lists.newArrayList(); + for (DeterministicSubsettingLoadBalancerConfig config : configs) { + ResolvedAddresses resolvedAddresses = + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.copyOf(shuffledServers)) + .setLoadBalancingPolicyConfig(config) + .build(); + loadBalancer.acceptResolvedAddresses(resolvedAddresses); + verify(mockChildLb, atLeastOnce()).handleResolvedAddresses(resolvedAddrCaptor.capture()); + // Verify ChildLB is only getting subsetSize ResolvedAddresses each time + assertThat(resolvedAddrCaptor.getValue().getAddresses().size()).isEqualTo(subsetSize); + actual.add(resolvedAddrCaptor.getValue().getAddresses()); + } + List actualSorted = actual.get(0); + List actualUnsorted = actual.get(1); + + // We will sort, and then round 0 will shift from 0,1,2,3 to 3,0,1,2 + assertThat(actualSorted).isEqualTo(Lists.newArrayList(servers.get(3), servers.get(0))); + // We will not sort, but round 0 will shift from 1,3,2,0 to 0,1,3,2 (same order given indices) + assertThat(actualUnsorted).isEqualTo(Lists.newArrayList(servers.get(0), servers.get(1))); + } + @Test public void closesUnusedConns() { addMock(); @@ -308,6 +423,13 @@ public void excludedBackendsInEveryRound() { verifyCreatesSubsets(21, 8, 5, 1); } + @Test + public void excludedStartBiggerThanEnd() { + // There are 3 excluded backends on each round, and sometimes the selected excluded backends + // wrap around. + verifyCreatesSubsets(7, 3, 4, 1); + } + public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int maxDiff) { setupBackends(backends); List configs = Lists.newArrayList(); @@ -343,6 +465,6 @@ public void verifyCreatesSubsets(int backends, int clients, int subsetSize, int int maxConns = Collections.max(subsetDistn.values()); int minConns = Collections.min(subsetDistn.values()); - assertThat(maxConns < minConns + maxConns).isTrue(); + assertThat(maxConns <= minConns + maxDiff).isTrue(); } } From 706c6f0da20eebe4a1e6892f793c1d92554f0820 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Fri, 18 Aug 2023 14:34:29 -0400 Subject: [PATCH 23/25] formatting --- ...ticSubsettingLoadBalancerProviderTest.java | 24 +++-- ...terministicSubsettingLoadBalancerTest.java | 97 ++++++++++--------- 2 files changed, 64 insertions(+), 57 deletions(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index d2864c6b2bf..9f3a97db6b2 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -28,7 +28,6 @@ import io.grpc.internal.JsonParser; import io.grpc.util.DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig; import java.io.IOException; -import java.util.ArrayList; import java.util.Map; import org.junit.Test; import org.junit.runner.RunWith; @@ -65,8 +64,12 @@ public void parseConfigRequiresClientIdx() throws IOException { ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(config)); assertThat(configOrError.getError()).isNotNull(); - assertThat(configOrError.getError().toString()).isEqualTo(Status.INTERNAL.withDescription( - "No client index set, cannot determine subsets {childPolicy=[{round_robin={}}]}" ).toString()); + assertThat(configOrError.getError().toString()) + .isEqualTo( + Status.INTERNAL + .withDescription( + "No client index set, cannot determine subsets {childPolicy=[{round_robin={}}]}") + .toString()); } @Test @@ -93,19 +96,20 @@ public void parseConfigWithDefaults() throws IOException { private static Map parseJsonObject(String json) throws IOException { return (Map) JsonParser.parse(json); } - + @Test public void parseConfigWithCustomSubsetSize() throws IOException { String lbConfig = - "{ \"clientIndex\" : 0, " - + "\"subsetSize\" : 3, " - + "\"childPolicy\" : [{\"round_robin\" : {}}], " - + "\"sortAddresses\" : false }"; + "{ \"clientIndex\" : 0, " + + "\"subsetSize\" : 3, " + + "\"childPolicy\" : [{\"round_robin\" : {}}], " + + "\"sortAddresses\" : false }"; - ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); assertThat(configOrError.getConfig()).isNotNull(); DeterministicSubsettingLoadBalancerConfig config = - (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); + (DeterministicSubsettingLoadBalancerConfig) configOrError.getConfig(); assertThat(config.subsetSize).isEqualTo(3); } } diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java index 8ced4dea8d6..3c08c744077 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerTest.java @@ -47,7 +47,6 @@ import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Random; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -70,10 +69,8 @@ public class DeterministicSubsettingLoadBalancerTest { @Mock private LoadBalancer.Helper mockHelper; @Mock private LoadBalancer mockChildLb; @Mock private SocketAddress mockSocketAddress; - @Captor - private ArgumentCaptor connectivityStateCaptor; - @Captor - private ArgumentCaptor errorPickerCaptor; + @Captor private ArgumentCaptor connectivityStateCaptor; + @Captor private ArgumentCaptor errorPickerCaptor; @Captor private ArgumentCaptor resolvedAddrCaptor; @@ -138,23 +135,26 @@ public Void answer(InvocationOnMock invocation) throws Throwable { public void handleNameResoutionError_noChildLb() { loadBalancer.handleNameResolutionError(Status.DEADLINE_EXCEEDED); - verify(mockHelper).updateBalancingState(connectivityStateCaptor.capture(), - errorPickerCaptor.capture()); + verify(mockHelper) + .updateBalancingState(connectivityStateCaptor.capture(), errorPickerCaptor.capture()); assertThat(connectivityStateCaptor.getValue()).isEqualTo(ConnectivityState.TRANSIENT_FAILURE); } + @Test public void handleNameResolutionError_withChildLb() { - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(2) - .setClientIndex(0) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) - .build(); - - loadBalancer.acceptResolvedAddresses(ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) - .setLoadBalancingPolicyConfig(config) - .build()); + DeterministicSubsettingLoadBalancerConfig config = + new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(2) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config) + .build()); loadBalancer.handleNameResolutionError(Status.DEADLINE_EXCEEDED); verify(mockChildLb).handleNameResolutionError(Status.DEADLINE_EXCEEDED); @@ -162,17 +162,19 @@ public void handleNameResolutionError_withChildLb() { @Test public void shutdown() { - DeterministicSubsettingLoadBalancerConfig config = new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(2) - .setClientIndex(0) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) - .build(); - - loadBalancer.acceptResolvedAddresses(ResolvedAddresses.newBuilder() - .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) - .setLoadBalancingPolicyConfig(config) - .build()); + DeterministicSubsettingLoadBalancerConfig config = + new DeterministicSubsettingLoadBalancer.DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(2) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + loadBalancer.acceptResolvedAddresses( + ResolvedAddresses.newBuilder() + .setAddresses(ImmutableList.of(new EquivalentAddressGroup(mockSocketAddress))) + .setLoadBalancingPolicyConfig(config) + .build()); loadBalancer.shutdown(); verify(mockChildLb).shutdown(); } @@ -181,13 +183,13 @@ public void shutdown() { public void addressComparator() { setupBackends(5); List sorted = Lists.newArrayList(); - for (EquivalentAddressGroup eag: servers) { + for (EquivalentAddressGroup eag : servers) { sorted.addAll(eag.getAddresses()); } Collections.shuffle(servers); List addresses = Lists.newArrayList(); - for (EquivalentAddressGroup eag: servers) { + for (EquivalentAddressGroup eag : servers) { addresses.addAll(eag.getAddresses()); } @@ -260,26 +262,27 @@ public void acceptResolvedAddresses() { public void sortingBackends() { setupBackends(4); // Shuffle servers so that they're not in 0, 1, 2 order - List shuffledServers = Lists.newArrayList( - servers.get(1), servers.get(3), servers.get(2), servers.get(0)); + List shuffledServers = + Lists.newArrayList(servers.get(1), servers.get(3), servers.get(2), servers.get(0)); int subsetSize = 2; DeterministicSubsettingLoadBalancerConfig sortConfig = - new DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(subsetSize) - .setClientIndex(0) - .setSortAddresses(true) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) - .build(); + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(true) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); DeterministicSubsettingLoadBalancerConfig dontSortConfig = - new DeterministicSubsettingLoadBalancerConfig.Builder() - .setSubsetSize(subsetSize) - .setClientIndex(0) - .setSortAddresses(false) - .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) - .build(); - - List configs = Lists.newArrayList(sortConfig, dontSortConfig); + new DeterministicSubsettingLoadBalancerConfig.Builder() + .setSubsetSize(subsetSize) + .setClientIndex(0) + .setSortAddresses(false) + .setChildPolicy(new PolicySelection(mockChildLbProvider, null)) + .build(); + + List configs = + Lists.newArrayList(sortConfig, dontSortConfig); List> actual = Lists.newArrayList(); for (DeterministicSubsettingLoadBalancerConfig config : configs) { ResolvedAddresses resolvedAddresses = From ab30f978d2fa21aa4b09a034ea89ddf33834c627 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Fri, 18 Aug 2023 15:49:03 -0400 Subject: [PATCH 24/25] checkstyle --- .../util/DeterministicSubsettingLoadBalancerProviderTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index 9f3a97db6b2..e8e41197300 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -68,7 +68,8 @@ public void parseConfigRequiresClientIdx() throws IOException { .isEqualTo( Status.INTERNAL .withDescription( - "No client index set, cannot determine subsets {childPolicy=[{round_robin={}}]}") + "No client index set, cannot determine subsets " + + " {childPolicy=[{round_robin={}}]}") .toString()); } From 02433be3c2cf18b7c09323bbf7ab592a64056bb8 Mon Sep 17 00:00:00 2001 From: Joy Bestourous Date: Fri, 18 Aug 2023 16:21:48 -0400 Subject: [PATCH 25/25] stray space --- .../util/DeterministicSubsettingLoadBalancerProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java index e8e41197300..36ead450bea 100644 --- a/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java +++ b/util/src/test/java/io/grpc/util/DeterministicSubsettingLoadBalancerProviderTest.java @@ -69,7 +69,7 @@ public void parseConfigRequiresClientIdx() throws IOException { Status.INTERNAL .withDescription( "No client index set, cannot determine subsets " - + " {childPolicy=[{round_robin={}}]}") + + "{childPolicy=[{round_robin={}}]}") .toString()); }