Simplify calculating delay after config failures

When there are several subscribers to a config and 1 JRTConfiguRequester
the counter for failures might quickly increase so backoff based
on these numbers will not be useful. Simplify by using booleans
for failures instead. Max delay for configured subscriptions will
now be 30 seconds instead of 150 seconds (which is way too much)

3 files changed, 76 insertions, 123 deletions

diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java
index efb93c6aed2..6878397a403 100644
--- a/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java
+++ b/config/src/main/java/com/yahoo/config/subscription/impl/JRTConfigRequester.java
@@ -44,8 +44,8 @@ public class JRTConfigRequester implements RequestWaiter {
     private static final JRTManagedConnectionPools managedPool = new JRTManagedConnectionPools();
     private static final int TRACELEVEL = 6;
     private final TimingValues timingValues;
-    private int fatalFailures = 0; // independent of transientFailures
-    private int transientFailures = 0;  // independent of fatalFailures
+    private boolean fatalFailures = false;
+    private boolean transientFailures = false;
     private final ScheduledThreadPoolExecutor scheduler;
     private Instant noApplicationWarningLogged = Instant.MIN;
     private static final Duration delayBetweenWarnings = Duration.ofSeconds(60);
@@ -173,19 +173,21 @@ public class JRTConfigRequester implements RequestWaiter {
         }
     }
 
-    static long calculateFailedRequestDelay(ErrorType errorCode, int transientFailures, int fatalFailures,
+    static long calculateFailedRequestDelay(ErrorType errorType, boolean transientFailures, boolean fatalFailures,
                                             TimingValues timingValues, boolean configured) {
-        long delay;
-        if (configured)
-            delay = timingValues.getConfiguredErrorDelay();
-        else
-            delay = timingValues.getUnconfiguredDelay();
-
-        if (errorCode == ErrorType.TRANSIENT) {
-            delay = delay * Math.min((transientFailures + 1), timingValues.getMaxDelayMultiplier());
-        } else {
-            delay = timingValues.getFixedDelay() + (delay * Math.min(fatalFailures, timingValues.getMaxDelayMultiplier()));
-            delay = timingValues.getPlusMinusFractionRandom(delay, randomFraction);
+        long delay = (configured ? timingValues.getConfiguredErrorDelay(): timingValues.getUnconfiguredDelay());
+
+        switch (errorType) {
+            case TRANSIENT:
+                if (transientFailures)
+                    delay = delay * 2;
+                break;
+            case FATAL:
+                delay = timingValues.getFixedDelay() + (delay * (fatalFailures ? 1 : 0));
+                delay = timingValues.getPlusMinusFractionRandom(delay, randomFraction);
+                break;
+            default:
+                throw new IllegalArgumentException("Unknown error type " + errorType);
         }
         return delay;
     }
@@ -194,7 +196,7 @@ public class JRTConfigRequester implements RequestWaiter {
                                          JRTConfigSubscription<ConfigInstance> sub,
                                          long delay,
                                          Connection connection) {
-        transientFailures++;
+        transientFailures = true;
         log.log(INFO, "Connection to " + connection.getAddress() +
                       " failed or timed out, clients will keep existing config, will keep trying.");
         if (sub.getState() != ConfigSubscription.State.OPEN) return;
@@ -217,7 +219,7 @@ public class JRTConfigRequester implements RequestWaiter {
     private void handleFatallyFailed(JRTClientConfigRequest jrtReq,
                                      JRTConfigSubscription<ConfigInstance> sub, long delay) {
         if (sub.getState() != ConfigSubscription.State.OPEN) return;
-        fatalFailures++;
+        fatalFailures = true;
         // The logging depends on whether we are configured or not.
         Level logLevel = sub.getConfigState().getConfig() == null ? Level.FINE : Level.INFO;
         String logMessage = "Request for config " + jrtReq.getShortDescription() + "' failed with error code " +
@@ -230,9 +232,8 @@ public class JRTConfigRequester implements RequestWaiter {
     private void handleOKRequest(JRTClientConfigRequest jrtReq,
                                  JRTConfigSubscription<ConfigInstance> sub,
                                  Connection connection) {
-        // Reset counters pertaining to error handling here
-        fatalFailures = 0;
-        transientFailures = 0;
+        fatalFailures = false;
+        transientFailures = false;
         noApplicationWarningLogged = Instant.MIN;
         sub.setLastCallBackOKTS(Instant.now());
         log.log(FINE, () -> "OK response received in handleOkRequest: " + jrtReq);
@@ -302,11 +303,11 @@ public class JRTConfigRequester implements RequestWaiter {
         }
     }
 
-    int getTransientFailures() {
+    boolean getTransientFailures() {
         return transientFailures;
     }
 
-    int getFatalFailures() {
+    boolean getFatalFailures() {
         return fatalFailures;
     }
 
diff --git a/config/src/main/java/com/yahoo/vespa/config/TimingValues.java b/config/src/main/java/com/yahoo/vespa/config/TimingValues.java
index 5d5967e56c4..fb7ef03e918 100644
--- a/config/src/main/java/com/yahoo/vespa/config/TimingValues.java
+++ b/config/src/main/java/com/yahoo/vespa/config/TimingValues.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.config;
 
 import java.util.Random;
@@ -21,7 +21,6 @@ public class TimingValues {
     private long fixedDelay = 5000;
     private long unconfiguredDelay = 1000;
     private long configuredErrorDelay = 15000;
-    private int  maxDelayMultiplier = 10;
     private final Random rand;
 
     public TimingValues() {
@@ -38,8 +37,7 @@ public class TimingValues {
                         long subscribeTimeout,
                         long unconfiguredDelay,
                         long configuredErrorDelay,
-                        long fixedDelay,
-                        int maxDelayMultiplier) {
+                        long fixedDelay) {
         this.successTimeout = successTimeout;
         this.errorTimeout = errorTimeout;
         this.initialTimeout = initialTimeout;
@@ -47,7 +45,6 @@ public class TimingValues {
         this.unconfiguredDelay = unconfiguredDelay;
         this.configuredErrorDelay = configuredErrorDelay;
         this.fixedDelay = fixedDelay;
-        this.maxDelayMultiplier = maxDelayMultiplier;
         this.rand = new Random(System.currentTimeMillis());
     }
 
@@ -58,7 +55,6 @@ public class TimingValues {
                          long unconfiguredDelay,
                          long configuredErrorDelay,
                          long fixedDelay,
-                         int maxDelayMultiplier,
                          Random rand) {
         this.successTimeout = successTimeout;
         this.errorTimeout = errorTimeout;
@@ -67,7 +63,6 @@ public class TimingValues {
         this.unconfiguredDelay = unconfiguredDelay;
         this.configuredErrorDelay = configuredErrorDelay;
         this.fixedDelay = fixedDelay;
-        this.maxDelayMultiplier = maxDelayMultiplier;
         this.rand = rand;
     }
 
@@ -79,7 +74,6 @@ public class TimingValues {
                 tv.unconfiguredDelay,
                 tv.configuredErrorDelay,
                 tv.fixedDelay,
-                tv.maxDelayMultiplier,
                 random);
     }
 
@@ -154,16 +148,6 @@ public class TimingValues {
     }
 
     /**
-     * Returns maximum multiplier to use when calculating delay (the delay is multiplied by the number of
-     * failed requests, unless that number is this maximum multiplier).
-     *
-     * @return timeout in milliseconds.
-     */
-    public int getMaxDelayMultiplier() {
-        return maxDelayMultiplier;
-    }
-
-    /**
      * Returns fixed delay that is used when retrying getting config no matter if it was a success or an error
      * and independent of number of retries.
      *
@@ -187,13 +171,14 @@ public class TimingValues {
     @Override
     public String toString() {
         return "TimingValues [successTimeout=" + successTimeout
-                + ", errorTimeout=" + errorTimeout + ", initialTimeout="
-                + initialTimeout + ", subscribeTimeout=" + subscribeTimeout
-                + ", configuredErrorTimeout=" + configuredErrorTimeout
-                + ", fixedDelay=" + fixedDelay + ", unconfiguredDelay="
-                + unconfiguredDelay + ", configuredErrorDelay="
-                + configuredErrorDelay + ", maxDelayMultiplier="
-                + maxDelayMultiplier + ", rand=" + rand + "]";
+               + ", errorTimeout=" + errorTimeout
+               + ", initialTimeout=" + initialTimeout
+               + ", subscribeTimeout=" + subscribeTimeout
+               + ", configuredErrorTimeout=" + configuredErrorTimeout
+               + ", fixedDelay=" + fixedDelay
+               + ", unconfiguredDelay=" + unconfiguredDelay
+               + ", configuredErrorDelay=" + configuredErrorDelay
+               + ", rand=" + rand + "]";
     }
 
 
diff --git a/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java b/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java
index 1b56e9290b2..658404f1772 100644
--- a/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java
+++ b/config/src/test/java/com/yahoo/config/subscription/impl/JRTConfigRequesterTest.java
@@ -1,9 +1,9 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.config.subscription.impl;
 
 import com.yahoo.config.subscription.ConfigSourceSet;
-import com.yahoo.foo.SimpletypesConfig;
 import com.yahoo.config.subscription.ConfigSubscriber;
+import com.yahoo.foo.SimpletypesConfig;
 import com.yahoo.jrt.Request;
 import com.yahoo.vespa.config.ConfigKey;
 import com.yahoo.vespa.config.ConnectionPool;
@@ -17,11 +17,11 @@ import java.util.Arrays;
 import java.util.List;
 import java.util.Random;
 
-import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -36,99 +36,68 @@ public class JRTConfigRequesterTest {
         TimingValues timingValues = new TimingValues(defaultTimingValues, random);
 
         // transientFailures and fatalFailures are not set until after delay has been calculated,
-        // so 0 is the case for the first failure
-        int transientFailures = 0;
-        int fatalFailures = 0;
+        // so false is the case for the first failure
+        boolean transientFailures = false;
+        boolean fatalFailures = false;
         boolean configured = false;
 
         // First time failure, not configured
         long delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is(timingValues.getUnconfiguredDelay()));
-        transientFailures = 5;
+        assertEquals(timingValues.getUnconfiguredDelay(), delay);
+        transientFailures = true;
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is((transientFailures + 1) * timingValues.getUnconfiguredDelay()));
-        transientFailures = 0;
+        assertEquals(timingValues.getUnconfiguredDelay() * 2, delay);
+        transientFailures = false;
 
 
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.FATAL,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertTrue(delay > (1 - JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
-        assertTrue(delay < (1 + JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
-        assertThat(delay, is(5462L));
+        assertTrue("delay=" + delay, delay > (1 - JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
+        assertTrue("delay=" + delay,delay < (1 + JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
+        assertEquals(5462, delay);
 
         // First time failure, configured
         configured = true;
-
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is(timingValues.getConfiguredErrorDelay()));
+        assertEquals(timingValues.getConfiguredErrorDelay(), delay);
 
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.FATAL,
                 transientFailures, fatalFailures, timingValues, configured);
         assertTrue(delay > (1 - JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
         assertTrue(delay < (1 + JRTConfigRequester.randomFraction) * timingValues.getFixedDelay());
-        assertThat(delay, is(5663L));
+        assertEquals(5663, delay);
 
 
         // nth time failure, not configured
-        fatalFailures = 1;
+        fatalFailures = true;
         configured = false;
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is(timingValues.getUnconfiguredDelay()));
+        assertEquals(timingValues.getUnconfiguredDelay(), delay);
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.FATAL,
                 transientFailures, fatalFailures, timingValues, configured);
         final long l = timingValues.getFixedDelay() + timingValues.getUnconfiguredDelay();
         assertTrue(delay > (1 - JRTConfigRequester.randomFraction) * l);
         assertTrue(delay < (1 + JRTConfigRequester.randomFraction) * l);
-        assertThat(delay, is(5377L));
+        assertEquals(5377, delay);
 
 
         // nth time failure, configured
-        fatalFailures = 1;
+        fatalFailures = true;
+        transientFailures = true;
         configured = true;
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
                 transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is(timingValues.getConfiguredErrorDelay()));
+        assertEquals(timingValues.getConfiguredErrorDelay() * 2, delay);
         delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.FATAL,
                 transientFailures, fatalFailures, timingValues, configured);
         final long l1 = timingValues.getFixedDelay() + timingValues.getConfiguredErrorDelay();
         assertTrue(delay > (1 - JRTConfigRequester.randomFraction) * l1);
         assertTrue(delay < (1 + JRTConfigRequester.randomFraction) * l1);
-        assertThat(delay, is(20851L));
-
-
-        // 1 more than max delay multiplier time failure, configured
-        fatalFailures = timingValues.getMaxDelayMultiplier() + 1;
-        configured = true;
-        delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
-                transientFailures, fatalFailures, timingValues, configured);
-        assertThat(delay, is(timingValues.getConfiguredErrorDelay()));
-        assertTrue(delay < timingValues.getMaxDelayMultiplier() * timingValues.getConfiguredErrorDelay());
-        delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.FATAL,
-                transientFailures, fatalFailures, timingValues, configured);
-        final long l2 = timingValues.getFixedDelay() + timingValues.getMaxDelayMultiplier() * timingValues.getConfiguredErrorDelay();
-        assertTrue(delay > (1 - JRTConfigRequester.randomFraction) * l2);
-        assertTrue(delay < (1 + JRTConfigRequester.randomFraction) * l2);
-        assertThat(delay, is(163520L));
-    }
-
-    @Test
-    public void testDelay() {
-        TimingValues timingValues = new TimingValues();
-
-        // transientFailures and fatalFailures are not set until after delay has been calculated,
-        // so 0 is the case for the first failure
-        int transientFailures = 0;
-        int fatalFailures = 0;
-
-        // First time failure, configured
-        long delay = JRTConfigRequester.calculateFailedRequestDelay(ErrorType.TRANSIENT,
-                transientFailures, fatalFailures, timingValues, true);
-        assertThat(delay, is(timingValues.getConfiguredErrorDelay()));
-        assertThat(delay, is((transientFailures + 1) * timingValues.getConfiguredErrorDelay()));
+        assertEquals(20851L, delay);
     }
 
     @Test
@@ -139,10 +108,10 @@ public class JRTConfigRequesterTest {
                 ErrorCode.ILLEGAL_DEF_MD5, ErrorCode.ILLEGAL_CONFIG_MD5, ErrorCode.ILLEGAL_TIMEOUT, ErrorCode.INTERNAL_ERROR,
                 9999); // unknown should also be fatal
         for (Integer i : transientErrors) {
-            assertThat(ErrorType.getErrorType(i), is(ErrorType.TRANSIENT));
+            assertEquals(ErrorType.TRANSIENT, ErrorType.getErrorType(i));
         }
         for (Integer i : fatalErrors) {
-            assertThat(ErrorType.getErrorType(i), is(ErrorType.FATAL));
+            assertEquals(ErrorType.FATAL, ErrorType.getErrorType(i));
         }
     }
 
@@ -154,16 +123,16 @@ public class JRTConfigRequesterTest {
 
         final MockConnection connection = new MockConnection();
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
-        assertThat(requester.getConnectionPool(), is(connection));
+        assertEquals(requester.getConnectionPool(), connection);
         requester.request(sub);
         final Request request = connection.getRequest();
         assertNotNull(request);
-        assertThat(connection.getNumberOfRequests(), is(1));
+        assertEquals(1, connection.getNumberOfRequests());
         JRTServerConfigRequestV3 receivedRequest = JRTServerConfigRequestV3.createFromRequest(request);
         assertTrue(receivedRequest.validateParameters());
-        assertThat(receivedRequest.getTimeout(), is(timingValues.getSubscribeTimeout()));
-        assertThat(requester.getFatalFailures(), is(0));
-        assertThat(requester.getTransientFailures(), is(0));
+        assertEquals(timingValues.getSubscribeTimeout(), receivedRequest.getTimeout());
+        assertFalse(requester.getFatalFailures());
+        assertFalse(requester.getTransientFailures());
     }
 
     @Test
@@ -175,8 +144,8 @@ public class JRTConfigRequesterTest {
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
         requester.request(createSubscription(subscriber, timingValues));
         waitUntilResponse(connection);
-        assertThat(requester.getFatalFailures(), is(1));
-        assertThat(requester.getTransientFailures(), is(0));
+        assertTrue(requester.getFatalFailures());
+        assertFalse(requester.getTransientFailures());;
     }
 
     @Test
@@ -190,8 +159,8 @@ public class JRTConfigRequesterTest {
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
         requester.request(sub);
         waitUntilResponse(connection);
-        assertThat(requester.getFatalFailures(), is(1));
-        assertThat(requester.getTransientFailures(), is(0));
+        assertTrue(requester.getFatalFailures());
+        assertFalse(requester.getTransientFailures());;
     }
 
     @Test
@@ -203,8 +172,8 @@ public class JRTConfigRequesterTest {
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
         requester.request(createSubscription(subscriber, timingValues));
         waitUntilResponse(connection);
-        assertThat(requester.getFatalFailures(), is(0));
-        assertThat(requester.getTransientFailures(), is(1));
+        assertFalse(requester.getFatalFailures());
+        assertTrue(requester.getTransientFailures());
     }
 
     @Test
@@ -218,8 +187,8 @@ public class JRTConfigRequesterTest {
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
         requester.request(sub);
         waitUntilResponse(connection);
-        assertThat(requester.getFatalFailures(), is(0));
-        assertThat(requester.getTransientFailures(), is(1));
+        assertFalse(requester.getFatalFailures());
+        assertTrue(requester.getTransientFailures());
     }
 
     @Test
@@ -231,12 +200,11 @@ public class JRTConfigRequesterTest {
 
         final MockConnection connection = new MockConnection(new ErrorResponseHandler(ErrorCode.UNKNOWN_DEFINITION));
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
-        assertThat(requester.getConnectionPool(), is(connection));
+        assertEquals(requester.getConnectionPool(), connection);
         requester.request(sub);
         waitUntilResponse(connection);
-        assertThat(requester.getFatalFailures(), is(1));
-        assertThat(requester.getTransientFailures(), is(0));
-        // TODO Check that no further request was sent?
+        assertTrue(requester.getFatalFailures());
+        assertFalse(requester.getTransientFailures());;
     }
 
     @Test
@@ -249,14 +217,14 @@ public class JRTConfigRequesterTest {
         final MockConnection connection = new MockConnection(new MockConnection.OKResponseHandler());
         JRTConfigRequester requester = new JRTConfigRequester(connection, timingValues);
         requester.request(sub);
-        assertThat(connection.getNumberOfRequests(), is(1));
+        assertEquals(1, connection.getNumberOfRequests());
         // Check that no further request was sent?
         try {
             Thread.sleep(timingValues.getFixedDelay()*2);
         } catch (InterruptedException e) {
             e.printStackTrace();
         }
-        assertThat(connection.getNumberOfRequests(), is(1));
+        assertEquals(1, connection.getNumberOfRequests());
     }
 
     @Test
@@ -308,8 +276,7 @@ public class JRTConfigRequesterTest {
             2000,  // subscribeTimeout
             250,   // unconfiguredDelay
             500,   // configuredErrorDelay
-            250,   // fixedDelay
-            5);    // maxDelayMultiplier
+            250);   // fixedDelay
     }
 
     private static class ErrorResponseHandler extends MockConnection.OKResponseHandler {