From eb5d48c4161b87ccd5a3d064e6200d9a4b7aa9d1 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 21 Nov 2022 20:42:53 +0000 Subject: [PATCH 01/14] Limit ring hash max size to 4K --- .../xds/RingHashLoadBalancerProvider.java | 19 +++++++++------ .../xds/RingHashLoadBalancerProviderTest.java | 23 +++++++++++++++---- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java index 84c6dedf631..5cd2d427572 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java @@ -39,11 +39,11 @@ public final class RingHashLoadBalancerProvider extends LoadBalancerProvider { static final long DEFAULT_MIN_RING_SIZE = 1024L; // Same as ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE @VisibleForTesting - static final long DEFAULT_MAX_RING_SIZE = 8 * 1024 * 1024L; - // Maximum number of ring entries allowed. Setting this too large can result in slow - // ring construction and OOM error. - // Same as ClientXdsClient.MAX_RING_HASH_LB_POLICY_RING_SIZE - static final long MAX_RING_SIZE = 8 * 1024 * 1024L; + static final long DEFAULT_MAX_RING_SIZE = 4 * 1024L; + // An upper bound on max ring size to limit memory usage. + // TODO(apolcyn): make MAX_RING_SIZE_CAP configurable, ideally on a per-channel + // basis but possibly as a global. + static final long MAX_RING_SIZE_CAP = 4 * 1024L; private static final boolean enableRingHash = Strings.isNullOrEmpty(System.getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH")) @@ -79,8 +79,13 @@ public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalanc if (maxRingSize == null) { maxRingSize = DEFAULT_MAX_RING_SIZE; } - if (minRingSize <= 0 || maxRingSize <= 0 || minRingSize > maxRingSize - || maxRingSize > MAX_RING_SIZE) { + if (minRingSize > MAX_RING_SIZE_CAP) { + minRingSize = MAX_RING_SIZE_CAP; + } + if (maxRingSize > MAX_RING_SIZE_CAP) { + maxRingSize = MAX_RING_SIZE_CAP; + } + if (minRingSize <= 0 || maxRingSize <= 0 || minRingSize > maxRingSize) { return ConfigOrError.fromError(Status.UNAVAILABLE.withDescription( "Invalid 'mingRingSize'/'maxRingSize'")); } diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index 9f972eaef92..436bd59f430 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -116,16 +116,29 @@ public void parseLoadBalancingConfig_invalid_minGreaterThanMax() throws IOExcept } @Test - public void parseLoadBalancingConfig_invalid_ringTooLarge() throws IOException { - long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE + 1; + public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { + long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP + 1; String lbConfig = String.format(Locale.US, "{\"minRingSize\" : 10, \"maxRingSize\" : %d}", ringSize); ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); assertThat(configOrError.getError()).isNotNull(); - assertThat(configOrError.getError().getCode()).isEqualTo(Code.UNAVAILABLE); - assertThat(configOrError.getError().getDescription()) - .isEqualTo("Invalid 'mingRingSize'/'maxRingSize'"); + RingHashConfig config = (RingHashConfig) configOrError.getConfig(); + assertThat(config.minRingSize).isEqualTo(10); + assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + } + + @Test + public void parseLoadBalancingConfig_ringMinAndMaxTooLargeUsesCap() throws IOException { + long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP + 1; + String lbConfig = + String.format(Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + assertThat(configOrError.getError()).isNotNull(); + RingHashConfig config = (RingHashConfig) configOrError.getConfig(); + assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); } @Test From bfe87cdbc4eda5286a9c0361675d93d4237490d1 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 21 Nov 2022 21:16:53 +0000 Subject: [PATCH 02/14] fix style --- .../java/io/grpc/xds/RingHashLoadBalancerProviderTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index 436bd59f430..a247cfbbb68 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -132,7 +132,8 @@ public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { public void parseLoadBalancingConfig_ringMinAndMaxTooLargeUsesCap() throws IOException { long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP + 1; String lbConfig = - String.format(Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); + String.format( + Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); assertThat(configOrError.getError()).isNotNull(); From cc7e820c92830a2728c1f1fc912e12225d17c70b Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 21 Nov 2022 21:59:02 +0000 Subject: [PATCH 03/14] fix test --- .../java/io/grpc/xds/RingHashLoadBalancerProviderTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index a247cfbbb68..1c321a5f091 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -122,7 +122,7 @@ public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { String.format(Locale.US, "{\"minRingSize\" : 10, \"maxRingSize\" : %d}", ringSize); ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); - assertThat(configOrError.getError()).isNotNull(); + assertThat(configOrError.getConfig()).isNotNull(); RingHashConfig config = (RingHashConfig) configOrError.getConfig(); assertThat(config.minRingSize).isEqualTo(10); assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); @@ -136,7 +136,7 @@ public void parseLoadBalancingConfig_ringMinAndMaxTooLargeUsesCap() throws IOExc Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); ConfigOrError configOrError = provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); - assertThat(configOrError.getError()).isNotNull(); + assertThat(configOrError.getConfig()).isNotNull(); RingHashConfig config = (RingHashConfig) configOrError.getConfig(); assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); From cba4cd5a60a59b67416025345e011371fd3be5b3 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 22:04:42 +0000 Subject: [PATCH 04/14] Add ring hash options class --- .../java/io/grpc/xds/RingHashOptions.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 xds/src/main/java/io/grpc/xds/RingHashOptions.java diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java new file mode 100644 index 00000000000..5b8347fdb01 --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -0,0 +1,64 @@ +/* + * Copyright 2021 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.xds; + +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 static io.grpc.ConnectivityState.CONNECTING; +import static io.grpc.ConnectivityState.IDLE; +import static io.grpc.ConnectivityState.READY; +import static io.grpc.ConnectivityState.SHUTDOWN; +import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; + +import com.google.common.base.MoreObjects; +import com.google.common.collect.Sets; +import io.grpc.Attributes; +import io.grpc.ConnectivityState; +import io.grpc.ConnectivityStateInfo; +import io.grpc.EquivalentAddressGroup; +import io.grpc.InternalLogId; +import io.grpc.LoadBalancer; +import io.grpc.Status; +import io.grpc.SynchronizationContext; +import io.grpc.xds.XdsLogger.XdsLogLevel; +import io.grpc.xds.XdsSubchannelPickers.ErrorPicker; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Random; +import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; +import javax.annotation.Nullable; + +/** + * Utility class that provides a way to configure ring hash size limits. This is applicable + * for clients that use the ring hash load balancing policy. Note that ring + * hash size limits involve a tradeoff between client memory consumption and accuracy of + * backend weights used for load balancing. Also see https://github.com/grpc/proposal/pull/338. + */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") +public final class RingHashOptions { + private static volatile maxRingSizeCap = 4096; + + public static setMaxRingSizeCap(long cap) { + RingHashOptions.maxRingSizeCap = maxRingSizeCap; + } +} From 4a4885dcf58427122640aacf9cb591a9a41d7b34 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 22:43:47 +0000 Subject: [PATCH 05/14] revise code --- .../io/grpc/xds/RingHashLoadBalancerProvider.java | 10 ++++++---- xds/src/main/java/io/grpc/xds/RingHashOptions.java | 11 +++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java index 5cd2d427572..1ab2e508360 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java @@ -26,6 +26,7 @@ import io.grpc.Status; import io.grpc.internal.JsonUtil; import io.grpc.xds.RingHashLoadBalancer.RingHashConfig; +import io.grpc.xds.RingHashOptions; import java.util.Map; /** @@ -73,17 +74,18 @@ public String getPolicyName() { public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalancingPolicyConfig) { Long minRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "minRingSize"); Long maxRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "maxRingSize"); + long maxRingSizeCap = RingHashOptions.getMaxRingSizeCap(); if (minRingSize == null) { minRingSize = DEFAULT_MIN_RING_SIZE; } if (maxRingSize == null) { maxRingSize = DEFAULT_MAX_RING_SIZE; } - if (minRingSize > MAX_RING_SIZE_CAP) { - minRingSize = MAX_RING_SIZE_CAP; + if (minRingSize > maxRingSizeCap) { + minRingSize = maxRingSizeCap; } - if (maxRingSize > MAX_RING_SIZE_CAP) { - maxRingSize = MAX_RING_SIZE_CAP; + if (maxRingSize > maxRingSizeCap) { + maxRingSize = maxRingSizeCap; } if (minRingSize <= 0 || maxRingSize <= 0 || minRingSize > maxRingSize) { return ConfigOrError.fromError(Status.UNAVAILABLE.withDescription( diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index 5b8347fdb01..1ef4fc8d85b 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -56,9 +56,16 @@ */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") public final class RingHashOptions { - private static volatile maxRingSizeCap = 4096; + // Same as ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE + @VisibleForTesting + static final long MAX_RING_SIZE_CAP = 8 * 1024 * 1024L; - public static setMaxRingSizeCap(long cap) { + // Same as RingHashLoadBalancerProvider.DEFAULT_MAX_RING_SIZE + private static volatile long maxRingSizeCap = 4 * 1024L; + + public static setMaxRingSizeCap(long maxRingSizeCap) { + maxRingSizeCap = Math.max(1, maxRingSizeCap); + maxRingSizeCap = Math.min(MAX_RING_SIZE_CAP, maxRingSizeCap); RingHashOptions.maxRingSizeCap = maxRingSizeCap; } } From 3a6b59e740079e16a94907488c275f74a8ca58ec Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 23:22:27 +0000 Subject: [PATCH 06/14] add tests --- .../java/io/grpc/xds/RingHashOptions.java | 32 +++++++--- .../xds/RingHashLoadBalancerProviderTest.java | 61 ++++++++++++++++++- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index 1ef4fc8d85b..b529071632e 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -50,9 +50,9 @@ /** * Utility class that provides a way to configure ring hash size limits. This is applicable - * for clients that use the ring hash load balancing policy. Note that ring - * hash size limits involve a tradeoff between client memory consumption and accuracy of - * backend weights used for load balancing. Also see https://github.com/grpc/proposal/pull/338. + * for clients that use the ring hash load balancing policy. Note that size limits involve + * a tradeoff between client memory consumption and accuracy of load balancing weight + * representations. Also see https://github.com/grpc/proposal/pull/338. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") public final class RingHashOptions { @@ -61,11 +61,27 @@ public final class RingHashOptions { static final long MAX_RING_SIZE_CAP = 8 * 1024 * 1024L; // Same as RingHashLoadBalancerProvider.DEFAULT_MAX_RING_SIZE - private static volatile long maxRingSizeCap = 4 * 1024L; + private static volatile long ringSizeCap = 4 * 1024L; - public static setMaxRingSizeCap(long maxRingSizeCap) { - maxRingSizeCap = Math.max(1, maxRingSizeCap); - maxRingSizeCap = Math.min(MAX_RING_SIZE_CAP, maxRingSizeCap); - RingHashOptions.maxRingSizeCap = maxRingSizeCap; + /** + * Set the global limit for min and max ring hash sizes. Note that + * this limit is clamped between 1 and 8M, and attempts to set + * the limit lower or higher than that range will be silently + * moved to the nearest number within that range. Defaults initially + * to 4K. + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") + public static void setRingSizeCap(long ringSizeCap) { + ringSizeCap = Math.max(1, ringSizeCap); + ringSizeCap = Math.min(MAX_RING_SIZE_CAP, ringSizeCap); + RingHashOptions.ringSizeCap = ringSizeCap; + } + + /** + * Get the global limit for min and max ring hash sizes. + */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") + public static long getRingSizeCap() { + return RingHashOptions.ringSizeCap; } } diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index 1c321a5f091..51c000be86e 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -29,6 +29,7 @@ import io.grpc.SynchronizationContext; import io.grpc.internal.JsonParser; import io.grpc.xds.RingHashLoadBalancer.RingHashConfig; +import io.grpc.xds.RingHashOptions; import java.io.IOException; import java.lang.Thread.UncaughtExceptionHandler; import java.util.Locale; @@ -129,8 +130,28 @@ public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { } @Test - public void parseLoadBalancingConfig_ringMinAndMaxTooLargeUsesCap() throws IOException { - long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP + 1; + public void parseLoadBalancingConfig_ringCapCanBeRaised() throws IOException { + long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); + RingHashOptions.setRingSizeCap(RingHashOptions.MAX_RING_SIZE_CAP); + long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; + String lbConfig = + String.format( + Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + assertThat(configOrError.getConfig()).isNotNull(); + RingHashConfig config = (RingHashConfig) configOrError.getConfig(); + assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + // Reset to avoid affecting subsequent test cases + RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + } + + @Test + public void parseLoadBalancingConfig_ringCapIsClampedTo8M() throws IOException { + long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); + RingHashOptions.setRingSizeCap(RingHashOptions.MAX_RING_SIZE_CAP + 1); + long ringSize = RingHashOptions.MAX_RING_SIZE_CAP + 1; String lbConfig = String.format( Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); @@ -140,6 +161,42 @@ public void parseLoadBalancingConfig_ringMinAndMaxTooLargeUsesCap() throws IOExc RingHashConfig config = (RingHashConfig) configOrError.getConfig(); assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + // Reset to avoid affecting subsequent test cases + RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + } + + public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { + long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); + RingHashOptions.setRingSizeCap(1); + long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; + String lbConfig = + String.format( + Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + assertThat(configOrError.getConfig()).isNotNull(); + RingHashConfig config = (RingHashConfig) configOrError.getConfig(); + assertThat(config.minRingSize).isEqualTo(1); + assertThat(config.maxRingSize).isEqualTo(1); + // Reset to avoid affecting subsequent test cases + RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + } + + public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { + long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); + RingHashOptions.setRingSizeCap(0); + long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; + String lbConfig = + String.format( + Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); + ConfigOrError configOrError = + provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); + assertThat(configOrError.getConfig()).isNotNull(); + RingHashConfig config = (RingHashConfig) configOrError.getConfig(); + assertThat(config.minRingSize).isEqualTo(1); + assertThat(config.maxRingSize).isEqualTo(1); + // Reset to avoid affecting subsequent test cases + RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); } @Test From 09edbfc33863ca65a1d877ab83916d4e4faee84d Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 23:32:27 +0000 Subject: [PATCH 07/14] remove unused imports --- .../java/io/grpc/xds/RingHashOptions.java | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index b529071632e..63efce62f53 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -16,38 +16,6 @@ package io.grpc.xds; -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 static io.grpc.ConnectivityState.CONNECTING; -import static io.grpc.ConnectivityState.IDLE; -import static io.grpc.ConnectivityState.READY; -import static io.grpc.ConnectivityState.SHUTDOWN; -import static io.grpc.ConnectivityState.TRANSIENT_FAILURE; - -import com.google.common.base.MoreObjects; -import com.google.common.collect.Sets; -import io.grpc.Attributes; -import io.grpc.ConnectivityState; -import io.grpc.ConnectivityStateInfo; -import io.grpc.EquivalentAddressGroup; -import io.grpc.InternalLogId; -import io.grpc.LoadBalancer; -import io.grpc.Status; -import io.grpc.SynchronizationContext; -import io.grpc.xds.XdsLogger.XdsLogLevel; -import io.grpc.xds.XdsSubchannelPickers.ErrorPicker; -import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.Random; -import java.util.Set; -import java.util.concurrent.atomic.AtomicLong; -import javax.annotation.Nullable; - /** * Utility class that provides a way to configure ring hash size limits. This is applicable * for clients that use the ring hash load balancing policy. Note that size limits involve From 069e10005739a594c4f9b81a8f7b666a71d4cba8 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 23:45:49 +0000 Subject: [PATCH 08/14] add annotation --- xds/src/main/java/io/grpc/xds/RingHashOptions.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index 63efce62f53..1cee8f77b55 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -16,6 +16,8 @@ package io.grpc.xds; +import io.grpc.ExperimentalApi; + /** * Utility class that provides a way to configure ring hash size limits. This is applicable * for clients that use the ring hash load balancing policy. Note that size limits involve From 7f8dcf29c4ed59d8faab5ac60a9c601d1800046f Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 28 Nov 2022 23:52:12 +0000 Subject: [PATCH 09/14] add annotation --- xds/src/main/java/io/grpc/xds/RingHashOptions.java | 1 + 1 file changed, 1 insertion(+) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index 1cee8f77b55..df3aa50ea16 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -16,6 +16,7 @@ package io.grpc.xds; +import com.google.common.annotations.VisibleForTesting; import io.grpc.ExperimentalApi; /** From 1997c0047891f73e27d78e0d01477c0aba165c65 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Tue, 29 Nov 2022 00:04:46 +0000 Subject: [PATCH 10/14] fix build --- xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java index 1ab2e508360..577ac25f7c2 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java @@ -74,7 +74,7 @@ public String getPolicyName() { public ConfigOrError parseLoadBalancingPolicyConfig(Map rawLoadBalancingPolicyConfig) { Long minRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "minRingSize"); Long maxRingSize = JsonUtil.getNumberAsLong(rawLoadBalancingPolicyConfig, "maxRingSize"); - long maxRingSizeCap = RingHashOptions.getMaxRingSizeCap(); + long maxRingSizeCap = RingHashOptions.getRingSizeCap(); if (minRingSize == null) { minRingSize = DEFAULT_MIN_RING_SIZE; } From c6810dcea19c49561053af5a8e55bb7706a32d31 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Tue, 29 Nov 2022 00:27:52 +0000 Subject: [PATCH 11/14] Fix build --- .../io/grpc/xds/RingHashLoadBalancerProviderTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index 51c000be86e..ca4bd535e37 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -144,7 +144,7 @@ public void parseLoadBalancingConfig_ringCapCanBeRaised() throws IOException { assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); // Reset to avoid affecting subsequent test cases - RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } @Test @@ -162,7 +162,7 @@ public void parseLoadBalancingConfig_ringCapIsClampedTo8M() throws IOException { assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); // Reset to avoid affecting subsequent test cases - RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { @@ -179,7 +179,7 @@ public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { assertThat(config.minRingSize).isEqualTo(1); assertThat(config.maxRingSize).isEqualTo(1); // Reset to avoid affecting subsequent test cases - RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { @@ -196,7 +196,7 @@ public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { assertThat(config.minRingSize).isEqualTo(1); assertThat(config.maxRingSize).isEqualTo(1); // Reset to avoid affecting subsequent test cases - RingHashOptions.setMaxRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } @Test From f89ded1403db444b161ccb6189ae0a052247995f Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Tue, 29 Nov 2022 00:47:36 +0000 Subject: [PATCH 12/14] fix test annotation --- .../test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index ca4bd535e37..4bab21f5eca 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -165,6 +165,7 @@ public void parseLoadBalancingConfig_ringCapIsClampedTo8M() throws IOException { RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } + @Test public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(1); @@ -182,6 +183,7 @@ public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); } + @Test public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(0); From 244e005c7f8be02db93f0153023312a2b7327dfc Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Tue, 29 Nov 2022 01:27:38 +0000 Subject: [PATCH 13/14] fix defaults --- .../xds/RingHashLoadBalancerProvider.java | 4 --- .../java/io/grpc/xds/RingHashOptions.java | 7 +++-- .../xds/RingHashLoadBalancerProviderTest.java | 28 ++++++++----------- 3 files changed, 17 insertions(+), 22 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java index 577ac25f7c2..5bba0dc9b0a 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java +++ b/xds/src/main/java/io/grpc/xds/RingHashLoadBalancerProvider.java @@ -41,10 +41,6 @@ public final class RingHashLoadBalancerProvider extends LoadBalancerProvider { // Same as ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE @VisibleForTesting static final long DEFAULT_MAX_RING_SIZE = 4 * 1024L; - // An upper bound on max ring size to limit memory usage. - // TODO(apolcyn): make MAX_RING_SIZE_CAP configurable, ideally on a per-channel - // basis but possibly as a global. - static final long MAX_RING_SIZE_CAP = 4 * 1024L; private static final boolean enableRingHash = Strings.isNullOrEmpty(System.getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_RING_HASH")) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index df3aa50ea16..ab942654513 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -30,9 +30,12 @@ public final class RingHashOptions { // Same as ClientXdsClient.DEFAULT_RING_HASH_LB_POLICY_MAX_RING_SIZE @VisibleForTesting static final long MAX_RING_SIZE_CAP = 8 * 1024 * 1024L; - + @VisibleForTesting // Same as RingHashLoadBalancerProvider.DEFAULT_MAX_RING_SIZE - private static volatile long ringSizeCap = 4 * 1024L; + static final long DEFAULT_RING_SIZE_CAP = 4 * 1024L; + + // Limits ring hash sizes to restrict client memory usage. + private static volatile long ringSizeCap = DEFAULT_RING_SIZE_CAP; /** * Set the global limit for min and max ring hash sizes. Note that diff --git a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java index 4bab21f5eca..87615a125c0 100644 --- a/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/RingHashLoadBalancerProviderTest.java @@ -118,7 +118,7 @@ public void parseLoadBalancingConfig_invalid_minGreaterThanMax() throws IOExcept @Test public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { - long ringSize = RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP + 1; + long ringSize = RingHashOptions.MAX_RING_SIZE_CAP + 1; String lbConfig = String.format(Locale.US, "{\"minRingSize\" : 10, \"maxRingSize\" : %d}", ringSize); ConfigOrError configOrError = @@ -126,12 +126,11 @@ public void parseLoadBalancingConfig_ringTooLargeUsesCap() throws IOException { assertThat(configOrError.getConfig()).isNotNull(); RingHashConfig config = (RingHashConfig) configOrError.getConfig(); assertThat(config.minRingSize).isEqualTo(10); - assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + assertThat(config.maxRingSize).isEqualTo(RingHashOptions.DEFAULT_RING_SIZE_CAP); } @Test public void parseLoadBalancingConfig_ringCapCanBeRaised() throws IOException { - long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(RingHashOptions.MAX_RING_SIZE_CAP); long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; String lbConfig = @@ -141,15 +140,14 @@ public void parseLoadBalancingConfig_ringCapCanBeRaised() throws IOException { provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); assertThat(configOrError.getConfig()).isNotNull(); RingHashConfig config = (RingHashConfig) configOrError.getConfig(); - assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); - assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + assertThat(config.minRingSize).isEqualTo(RingHashOptions.MAX_RING_SIZE_CAP); + assertThat(config.maxRingSize).isEqualTo(RingHashOptions.MAX_RING_SIZE_CAP); // Reset to avoid affecting subsequent test cases - RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(RingHashOptions.DEFAULT_RING_SIZE_CAP); } @Test public void parseLoadBalancingConfig_ringCapIsClampedTo8M() throws IOException { - long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(RingHashOptions.MAX_RING_SIZE_CAP + 1); long ringSize = RingHashOptions.MAX_RING_SIZE_CAP + 1; String lbConfig = @@ -159,17 +157,16 @@ public void parseLoadBalancingConfig_ringCapIsClampedTo8M() throws IOException { provider.parseLoadBalancingPolicyConfig(parseJsonObject(lbConfig)); assertThat(configOrError.getConfig()).isNotNull(); RingHashConfig config = (RingHashConfig) configOrError.getConfig(); - assertThat(config.minRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); - assertThat(config.maxRingSize).isEqualTo(RingHashLoadBalancerProvider.MAX_RING_SIZE_CAP); + assertThat(config.minRingSize).isEqualTo(RingHashOptions.MAX_RING_SIZE_CAP); + assertThat(config.maxRingSize).isEqualTo(RingHashOptions.MAX_RING_SIZE_CAP); // Reset to avoid affecting subsequent test cases - RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(RingHashOptions.DEFAULT_RING_SIZE_CAP); } @Test public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { - long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(1); - long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; + long ringSize = 2; String lbConfig = String.format( Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); @@ -180,14 +177,13 @@ public void parseLoadBalancingConfig_ringCapCanBeLowered() throws IOException { assertThat(config.minRingSize).isEqualTo(1); assertThat(config.maxRingSize).isEqualTo(1); // Reset to avoid affecting subsequent test cases - RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(RingHashOptions.DEFAULT_RING_SIZE_CAP); } @Test public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { - long originalMaxRingSizeCap = RingHashOptions.getRingSizeCap(); RingHashOptions.setRingSizeCap(0); - long ringSize = RingHashOptions.MAX_RING_SIZE_CAP; + long ringSize = 2; String lbConfig = String.format( Locale.US, "{\"minRingSize\" : %d, \"maxRingSize\" : %d}", ringSize, ringSize); @@ -198,7 +194,7 @@ public void parseLoadBalancingConfig_ringCapLowerLimitIs1() throws IOException { assertThat(config.minRingSize).isEqualTo(1); assertThat(config.maxRingSize).isEqualTo(1); // Reset to avoid affecting subsequent test cases - RingHashOptions.setRingSizeCap(originalMaxRingSizeCap); + RingHashOptions.setRingSizeCap(RingHashOptions.DEFAULT_RING_SIZE_CAP); } @Test From dd4aa34965449004e31bcf69521dff8928aaea3d Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Tue, 29 Nov 2022 02:41:43 +0000 Subject: [PATCH 14/14] review feedback --- xds/src/main/java/io/grpc/xds/RingHashOptions.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RingHashOptions.java b/xds/src/main/java/io/grpc/xds/RingHashOptions.java index ab942654513..6bb3fc7887e 100644 --- a/xds/src/main/java/io/grpc/xds/RingHashOptions.java +++ b/xds/src/main/java/io/grpc/xds/RingHashOptions.java @@ -37,12 +37,13 @@ public final class RingHashOptions { // Limits ring hash sizes to restrict client memory usage. private static volatile long ringSizeCap = DEFAULT_RING_SIZE_CAP; + private RingHashOptions() {} // Prevent instantiation + /** - * Set the global limit for min and max ring hash sizes. Note that - * this limit is clamped between 1 and 8M, and attempts to set - * the limit lower or higher than that range will be silently - * moved to the nearest number within that range. Defaults initially - * to 4K. + * Set the global limit for the min and max number of ring hash entries per ring. + * Note that this limit is clamped between 1 entry and 8,388,608 entries, and new + * limits lying outside that range will be silently moved to the nearest number within + * that range. Defaults initially to 4096 entries. */ @ExperimentalApi("https://github.com/grpc/grpc-java/issues/9718") public static void setRingSizeCap(long ringSizeCap) {