From a40b686891920b825171ed90c81fc36d962e75d1 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Fri, 25 Mar 2016 10:23:58 -0700 Subject: [PATCH] Revert "Refactor ExponentialBackoffPolicy" This reverts commit a98f8afbbeef1d088e9765102926b6e479ab87a9. There was no expectation across the languages that we would support other policies for connection retry (changing a parameter would be on the table, though). --- .../io/grpc/ExponentialBackoffPolicy.java | 150 ------------------ .../java/io/grpc/ManagedChannelBuilder.java | 6 - .../AbstractManagedChannelImplBuilder.java | 15 +- .../io/grpc/{ => internal}/BackoffPolicy.java | 4 +- .../internal/ExponentialBackoffPolicy.java | 112 +++++++++++++ .../io/grpc/internal/ManagedChannelImpl.java | 1 - .../java/io/grpc/internal/TransportSet.java | 1 - .../ExponentialBackoffPolicyTest.java | 20 +-- .../grpc/internal/ManagedChannelImplTest.java | 1 - ...anagedChannelImplTransportManagerTest.java | 1 - .../io/grpc/internal/TransportSetTest.java | 1 - 11 files changed, 122 insertions(+), 190 deletions(-) delete mode 100644 core/src/main/java/io/grpc/ExponentialBackoffPolicy.java rename core/src/main/java/io/grpc/{ => internal}/BackoffPolicy.java (97%) create mode 100644 core/src/main/java/io/grpc/internal/ExponentialBackoffPolicy.java rename core/src/test/java/io/grpc/{ => internal}/ExponentialBackoffPolicyTest.java (86%) diff --git a/core/src/main/java/io/grpc/ExponentialBackoffPolicy.java b/core/src/main/java/io/grpc/ExponentialBackoffPolicy.java deleted file mode 100644 index e2fb624393..0000000000 --- a/core/src/main/java/io/grpc/ExponentialBackoffPolicy.java +++ /dev/null @@ -1,150 +0,0 @@ -/* - * Copyright 2015, Google Inc. All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ - -package io.grpc; - -import static com.google.common.base.MoreObjects.firstNonNull; -import static com.google.common.base.Preconditions.checkArgument; - -import java.util.Random; -import java.util.concurrent.TimeUnit; - -/** - * Exponential backoff policy for reconnects. - */ -public final class ExponentialBackoffPolicy implements BackoffPolicy { - /** - * Provider tuned for connection retries. - * https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md - */ - public static final class Provider implements BackoffPolicy.Provider { - @Override - public BackoffPolicy get() { - return ExponentialBackoffPolicy.builder() - .initialBackoffMilis(1, TimeUnit.SECONDS) - .maxBackoffMillis(2, TimeUnit.MINUTES) - .multiplier(1.6) - .jitter(.2) - .build(); - } - } - - private final Random random; - private final long initialBackoffMillis; - private final long maxBackoffMillis; - private final double multiplier; - private final double jitter; - - private long nextBackoffMillis; - - private ExponentialBackoffPolicy(Random random, long initialBackoffMilis, long maxBackoffMillis, - double multiplier, double jitter) { - this.random = random; - this.initialBackoffMillis = initialBackoffMilis; - this.maxBackoffMillis = maxBackoffMillis; - this.multiplier = multiplier; - this.jitter = jitter; - - this.nextBackoffMillis = initialBackoffMillis; - } - - @Override - public long nextBackoffMillis() { - long currentBackoffMillis = nextBackoffMillis; - nextBackoffMillis = Math.min((long) (currentBackoffMillis * multiplier), maxBackoffMillis); - return currentBackoffMillis - + uniformRandom(-jitter * currentBackoffMillis, jitter * currentBackoffMillis); - } - - private long uniformRandom(double low, double high) { - checkArgument(high >= low); - double mag = high - low; - return (long) (random.nextDouble() * mag + low); - } - - public static Builder builder() { - return new Builder(); - } - - public static final class Builder { - private Random random; - private long initialBackoffMilis; - private long maxBackoffMillis; - private double multiplier; - private double jitter; - - private Builder() { - } - - public Builder random(Random random) { - this.random = random; - return this; - } - - public Builder initialBackoffMilis(long initialBackoff, TimeUnit timeUnit) { - this.initialBackoffMilis = timeUnit.toMillis(initialBackoff); - return this; - } - - public Builder maxBackoffMillis(long maxBackoff, TimeUnit timeUnit) { - this.maxBackoffMillis = timeUnit.toMillis(maxBackoff); - return this; - } - - public Builder multiplier(double multiplier) { - this.multiplier = multiplier; - return this; - } - - public Builder jitter(double jitter) { - this.jitter = jitter; - return this; - } - - /** - * Builds {@link BackoffPolicy} instance. - * - * @return BackoffPolicy object. - */ - public ExponentialBackoffPolicy build() { - checkArgument(initialBackoffMilis >= 0, "initialBackoffMilis must be greater or equal zero"); - checkArgument(maxBackoffMillis > 0, "maxBackoffMillis must be greater than zero"); - checkArgument(initialBackoffMilis <= maxBackoffMillis, "initialBackoffMilis can't be greater" - + "or equal maxBackoffMillis"); - checkArgument(multiplier > 0, "multiplier must be greater than zero"); - checkArgument(jitter >= 0, "jitter must be greater or equal zero"); - - return new ExponentialBackoffPolicy(firstNonNull(random, new Random()), initialBackoffMilis, - maxBackoffMillis, multiplier, jitter); - } - } -} - diff --git a/core/src/main/java/io/grpc/ManagedChannelBuilder.java b/core/src/main/java/io/grpc/ManagedChannelBuilder.java index b5091b0e9d..3d74c9e588 100644 --- a/core/src/main/java/io/grpc/ManagedChannelBuilder.java +++ b/core/src/main/java/io/grpc/ManagedChannelBuilder.java @@ -179,12 +179,6 @@ public abstract class ManagedChannelBuilder> @ExperimentalApi public abstract T compressorRegistry(CompressorRegistry registry); - /** - * Provides a custom {@link BackoffPolicy.Provider} for reconnects. - */ - @ExperimentalApi - public abstract T backoffPolicyProvider(BackoffPolicy.Provider backoffPolicyProvider); - /** * Builds a channel using the given parameters. */ diff --git a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java index dcb36428d9..42e79ba8b0 100644 --- a/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java @@ -37,12 +37,10 @@ import com.google.common.base.Preconditions; import com.google.common.util.concurrent.MoreExecutors; import io.grpc.Attributes; -import io.grpc.BackoffPolicy; import io.grpc.ClientInterceptor; import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; import io.grpc.ExperimentalApi; -import io.grpc.ExponentialBackoffPolicy; import io.grpc.LoadBalancer; import io.grpc.ManagedChannelBuilder; import io.grpc.NameResolver; @@ -96,9 +94,6 @@ public abstract class AbstractManagedChannelImplBuilder @Nullable private CompressorRegistry compressorRegistry; - @Nullable - private BackoffPolicy.Provider backoffPolicyProvider; - protected AbstractManagedChannelImplBuilder(String target) { this.target = Preconditions.checkNotNull(target); this.directServerAddress = null; @@ -164,13 +159,6 @@ public abstract class AbstractManagedChannelImplBuilder return thisT(); } - @Override - @ExperimentalApi - public final T backoffPolicyProvider(BackoffPolicy.Provider backoffPolicyProvider) { - this.backoffPolicyProvider = backoffPolicyProvider; - return thisT(); - } - private T thisT() { @SuppressWarnings("unchecked") T thisT = (T) this; @@ -204,7 +192,8 @@ public abstract class AbstractManagedChannelImplBuilder buildTransportFactory(), authorityOverride); return new ManagedChannelImpl( target, - firstNonNull(backoffPolicyProvider, new ExponentialBackoffPolicy.Provider()), + // TODO(carl-mastrangelo): Allow clients to pass this in + new ExponentialBackoffPolicy.Provider(), firstNonNull(nameResolverFactory, NameResolverRegistry.getDefaultRegistry()), getNameResolverParams(), firstNonNull(loadBalancerFactory, SimpleLoadBalancerFactory.getInstance()), diff --git a/core/src/main/java/io/grpc/BackoffPolicy.java b/core/src/main/java/io/grpc/internal/BackoffPolicy.java similarity index 97% rename from core/src/main/java/io/grpc/BackoffPolicy.java rename to core/src/main/java/io/grpc/internal/BackoffPolicy.java index 00616690de..1a2bc375ea 100644 --- a/core/src/main/java/io/grpc/BackoffPolicy.java +++ b/core/src/main/java/io/grpc/internal/BackoffPolicy.java @@ -29,12 +29,12 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package io.grpc; +package io.grpc.internal; /** * Determines how long to wait before doing some action (typically a retry, or a reconnect). */ -public interface BackoffPolicy { +interface BackoffPolicy { interface Provider { BackoffPolicy get(); } diff --git a/core/src/main/java/io/grpc/internal/ExponentialBackoffPolicy.java b/core/src/main/java/io/grpc/internal/ExponentialBackoffPolicy.java new file mode 100644 index 0000000000..8d3191fb3b --- /dev/null +++ b/core/src/main/java/io/grpc/internal/ExponentialBackoffPolicy.java @@ -0,0 +1,112 @@ +/* + * Copyright 2015, Google Inc. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package io.grpc.internal; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.common.annotations.VisibleForTesting; + +import java.util.Random; +import java.util.concurrent.TimeUnit; + +/** + * Retry Policy for Transport reconnection. Initial parameters from + * https://github.com/grpc/grpc/blob/master/doc/connection-backoff.md + * + *

TODO(carl-mastrangelo): add unit tests for this class + */ +final class ExponentialBackoffPolicy implements BackoffPolicy { + static final class Provider implements BackoffPolicy.Provider { + @Override + public BackoffPolicy get() { + return new ExponentialBackoffPolicy(); + } + } + + private Random random = new Random(); + private long initialBackoffMillis = TimeUnit.SECONDS.toMillis(1); + private long maxBackoffMillis = TimeUnit.MINUTES.toMillis(2); + private double multiplier = 1.6; + private double jitter = .2; + + private long nextBackoffMillis = initialBackoffMillis; + + @Override + public long nextBackoffMillis() { + long currentBackoffMillis = nextBackoffMillis; + nextBackoffMillis = Math.min((long) (currentBackoffMillis * multiplier), maxBackoffMillis); + return currentBackoffMillis + + uniformRandom(-jitter * currentBackoffMillis, jitter * currentBackoffMillis); + } + + private long uniformRandom(double low, double high) { + checkArgument(high >= low); + double mag = high - low; + return (long) (random.nextDouble() * mag + low); + } + + /* + * No guice and no flags means we get to implement these setters for testing ourselves. Do not + * call these from non-test code. + */ + + @VisibleForTesting + ExponentialBackoffPolicy setRandom(Random random) { + this.random = random; + return this; + } + + @VisibleForTesting + ExponentialBackoffPolicy setInitialBackoffMillis(long initialBackoffMillis) { + this.initialBackoffMillis = initialBackoffMillis; + return this; + } + + @VisibleForTesting + ExponentialBackoffPolicy setMaxBackoffMillis(long maxBackoffMillis) { + this.maxBackoffMillis = maxBackoffMillis; + return this; + } + + @VisibleForTesting + ExponentialBackoffPolicy setMultiplier(double multiplier) { + this.multiplier = multiplier; + return this; + } + + @VisibleForTesting + ExponentialBackoffPolicy setJitter(double jitter) { + this.jitter = jitter; + return this; + } +} + diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 5d5c3826ca..0be3d7834b 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -38,7 +38,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Supplier; import io.grpc.Attributes; -import io.grpc.BackoffPolicy; import io.grpc.CallOptions; import io.grpc.Channel; import io.grpc.ClientCall; diff --git a/core/src/main/java/io/grpc/internal/TransportSet.java b/core/src/main/java/io/grpc/internal/TransportSet.java index 5f8142ba13..29228967ef 100644 --- a/core/src/main/java/io/grpc/internal/TransportSet.java +++ b/core/src/main/java/io/grpc/internal/TransportSet.java @@ -35,7 +35,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.base.Stopwatch; -import io.grpc.BackoffPolicy; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; import io.grpc.Status; diff --git a/core/src/test/java/io/grpc/ExponentialBackoffPolicyTest.java b/core/src/test/java/io/grpc/internal/ExponentialBackoffPolicyTest.java similarity index 86% rename from core/src/test/java/io/grpc/ExponentialBackoffPolicyTest.java rename to core/src/test/java/io/grpc/internal/ExponentialBackoffPolicyTest.java index ed40b2b402..af580329d6 100644 --- a/core/src/test/java/io/grpc/ExponentialBackoffPolicyTest.java +++ b/core/src/test/java/io/grpc/internal/ExponentialBackoffPolicyTest.java @@ -29,7 +29,7 @@ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ -package io.grpc; +package io.grpc.internal; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -39,13 +39,13 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import java.util.Random; -import java.util.concurrent.TimeUnit; /** * Test for {@link ExponentialBackoffPolicy}. */ @RunWith(JUnit4.class) public class ExponentialBackoffPolicyTest { + private ExponentialBackoffPolicy policy = new ExponentialBackoffPolicy(); private Random notRandom = new Random() { @Override public double nextDouble() { @@ -56,26 +56,18 @@ public class ExponentialBackoffPolicyTest { @Test public void maxDelayReached() { long maxBackoffMillis = 120 * 1000; - - ExponentialBackoffPolicy policy = ExponentialBackoffPolicy.builder() - .random(notRandom) - .initialBackoffMilis(1, TimeUnit.SECONDS) - .maxBackoffMillis(maxBackoffMillis, TimeUnit.MILLISECONDS) - .multiplier(1.6) - .jitter(0) - .build(); - + policy.setMaxBackoffMillis(maxBackoffMillis) + .setJitter(0) + .setRandom(notRandom); for (int i = 0; i < 50; i++) { if (maxBackoffMillis == policy.nextBackoffMillis()) { return; // Success } } - assertEquals("max delay not reached", maxBackoffMillis, policy.nextBackoffMillis()); } - @Test - public void canProvide() { + @Test public void canProvide() { assertNotNull(new ExponentialBackoffPolicy.Provider().get()); } } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 7df6af652c..7dd7d9c734 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -54,7 +54,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import io.grpc.Attributes; -import io.grpc.BackoffPolicy; import io.grpc.CallOptions; import io.grpc.Channel; import io.grpc.ClientCall; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java index 526eb9f5e3..3295e997ea 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java @@ -46,7 +46,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import io.grpc.Attributes; -import io.grpc.BackoffPolicy; import io.grpc.ClientInterceptor; import io.grpc.CompressorRegistry; import io.grpc.DecompressorRegistry; diff --git a/core/src/test/java/io/grpc/internal/TransportSetTest.java b/core/src/test/java/io/grpc/internal/TransportSetTest.java index 9257fd30f1..988b03a21f 100644 --- a/core/src/test/java/io/grpc/internal/TransportSetTest.java +++ b/core/src/test/java/io/grpc/internal/TransportSetTest.java @@ -47,7 +47,6 @@ import static org.mockito.Mockito.when; import com.google.common.base.Stopwatch; -import io.grpc.BackoffPolicy; import io.grpc.EquivalentAddressGroup; import io.grpc.IntegerMarshaller; import io.grpc.LoadBalancer;