Skip to content

Limit ring hash max size to 4K #9709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add tests
  • Loading branch information
apolcyn committed Nov 28, 2022
commit 3a6b59e740079e16a94907488c275f74a8ca58ec
32 changes: 24 additions & 8 deletions xds/src/main/java/io/grpc/xds/RingHashOptions.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down