Merge pull request #17626 from vespa-engine/hmusum/switch-only-when-current-connection-is-failing

Switch connection only when the current one is failing [run-systemtest]

11 files changed, 68 insertions, 127 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 f846231ac98..efb93c6aed2 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
@@ -234,7 +234,6 @@ public class JRTConfigRequester implements RequestWaiter {
         fatalFailures = 0;
         transientFailures = 0;
         noApplicationWarningLogged = Instant.MIN;
-        connection.setSuccess();
         sub.setLastCallBackOKTS(Instant.now());
         log.log(FINE, () -> "OK response received in handleOkRequest: " + jrtReq);
         if (jrtReq.hasUpdatedGeneration()) {
diff --git a/config/src/main/java/com/yahoo/config/subscription/impl/MockConnection.java b/config/src/main/java/com/yahoo/config/subscription/impl/MockConnection.java
index e6b81adf787..bed7a0fa3c4 100644
--- a/config/src/main/java/com/yahoo/config/subscription/impl/MockConnection.java
+++ b/config/src/main/java/com/yahoo/config/subscription/impl/MockConnection.java
@@ -22,11 +22,6 @@ public class MockConnection implements ConnectionPool, Connection {
     private final ResponseHandler responseHandler;
     private int numberOfRequests = 0;
 
-    public int getNumberOfFailovers() {
-        return numberOfFailovers;
-    }
-
-    private int numberOfFailovers = 0;
     private final int numSpecs;
 
     public MockConnection() {
@@ -59,16 +54,6 @@ public class MockConnection implements ConnectionPool, Connection {
     }
 
     @Override
-    public void setError(int errorCode) {
-        numberOfFailovers++;
-    }
-
-    @Override
-    public void setSuccess() {
-        numberOfFailovers = 0;
-    }
-
-    @Override
     public String getAddress() {
         return null;
     }
@@ -77,9 +62,7 @@ public class MockConnection implements ConnectionPool, Connection {
     public void close() {}
 
     @Override
-    public void setError(Connection connection, int errorCode) {
-        connection.setError(errorCode);
-    }
+    public void setError(Connection connection, int errorCode) { }
 
     @Override
     public Connection getCurrent() {
@@ -87,7 +70,7 @@ public class MockConnection implements ConnectionPool, Connection {
     }
 
     @Override
-    public Connection switchConnection() { return this; }
+    public Connection switchConnection(Connection connection) { return this; }
 
     @Override
     public int getSize() {
diff --git a/config/src/main/java/com/yahoo/vespa/config/Connection.java b/config/src/main/java/com/yahoo/vespa/config/Connection.java
index e39175a3a78..f10fbc487bd 100644
--- a/config/src/main/java/com/yahoo/vespa/config/Connection.java
+++ b/config/src/main/java/com/yahoo/vespa/config/Connection.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 com.yahoo.jrt.Request;
@@ -13,9 +13,6 @@ public interface Connection {
 
     void invokeSync(Request request, double jrtTimeout);
 
-    void setError(int errorCode);
-
-    void setSuccess();
-
     String getAddress();
+
 }
diff --git a/config/src/main/java/com/yahoo/vespa/config/ConnectionPool.java b/config/src/main/java/com/yahoo/vespa/config/ConnectionPool.java
index 786dfa975f4..93135fc4661 100644
--- a/config/src/main/java/com/yahoo/vespa/config/ConnectionPool.java
+++ b/config/src/main/java/com/yahoo/vespa/config/ConnectionPool.java
@@ -12,7 +12,7 @@ public interface ConnectionPool extends AutoCloseable {
 
     /**
      * Sets the supplied Connection to have an error, implementations are expected to call
-     * {@link #switchConnection()} after setting state for the supplied Connection.
+     * {@link #switchConnection(Connection)} after setting state for the supplied Connection.
      *
      */
     void setError(Connection connection, int i);
@@ -25,16 +25,7 @@ public interface ConnectionPool extends AutoCloseable {
      *
      * @return a Connection
      */
-    Connection switchConnection();
-
-    /**
-     * Sets the current JRTConnection instance by randomly choosing
-     * from the available sources and returns the result.
-     *
-     * @return a Connection
-     */
-    @Deprecated
-    default Connection setNewCurrentConnection() { return switchConnection(); };
+    Connection switchConnection(Connection failingConnection);
 
     int getSize();
 
diff --git a/config/src/main/java/com/yahoo/vespa/config/JRTConnection.java b/config/src/main/java/com/yahoo/vespa/config/JRTConnection.java
index 0d5f483ad2c..f6352204d4b 100644
--- a/config/src/main/java/com/yahoo/vespa/config/JRTConnection.java
+++ b/config/src/main/java/com/yahoo/vespa/config/JRTConnection.java
@@ -7,8 +7,7 @@ import com.yahoo.jrt.Spec;
 import com.yahoo.jrt.Supervisor;
 import com.yahoo.jrt.Target;
 
-import java.time.Duration;
-import java.time.Instant;
+import java.util.Objects;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -25,10 +24,6 @@ public class JRTConnection implements Connection {
     private final Supervisor supervisor;
     private Target target;
 
-    private Instant lastConnected = Instant.EPOCH.plus(Duration.ofSeconds(1)); // to be healthy initially, see isHealthy()
-    private Instant lastSuccess = Instant.EPOCH;
-    private Instant lastFailure = Instant.EPOCH;
-
     public JRTConnection(String address, Supervisor supervisor) {
         this.address = address;
         this.supervisor = supervisor;
@@ -58,38 +53,26 @@ public class JRTConnection implements Connection {
         if (target == null || !target.isValid()) {
             logger.log(Level.INFO, "Connecting to " + address);
             target = supervisor.connect(new Spec(address));
-            lastConnected = Instant.now();
         }
         return target;
     }
 
     @Override
-    public synchronized void setError(int errorCode) {
-        lastFailure = Instant.now();
+    public String toString() {
+        return address;
     }
 
     @Override
-    public synchronized void setSuccess() {
-        lastSuccess = Instant.now();
-    }
-
-    public synchronized boolean isHealthy() {
-        return lastSuccess.isAfter(lastFailure) || lastConnected.isAfter(lastFailure);
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        JRTConnection that = (JRTConnection) o;
+        return address.equals(that.address);
     }
 
-    public String toString() {
-        StringBuilder sb = new StringBuilder();
-        sb.append(address);
-        sb.append(", ").append(isHealthy() ? "healthy" : "unhealthy");
-        if (lastSuccess.isAfter(Instant.EPOCH)) {
-            sb.append(", last success: ");
-            sb.append(lastSuccess);
-        }
-        if (lastFailure.isAfter(Instant.EPOCH)) {
-            sb.append(", last failure: ");
-            sb.append(lastFailure);
-        }
-        return sb.toString();
+    @Override
+    public int hashCode() {
+        return Objects.hash(address);
     }
 
 }
diff --git a/config/src/main/java/com/yahoo/vespa/config/JRTConnectionPool.java b/config/src/main/java/com/yahoo/vespa/config/JRTConnectionPool.java
index 019103c7015..26eafb67c1b 100644
--- a/config/src/main/java/com/yahoo/vespa/config/JRTConnectionPool.java
+++ b/config/src/main/java/com/yahoo/vespa/config/JRTConnectionPool.java
@@ -12,7 +12,6 @@ import java.util.Map;
 import java.util.concurrent.ThreadLocalRandom;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 /**
  * A pool of JRT connections to a config source (either a config server or a config proxy).
@@ -66,19 +65,26 @@ public class JRTConnectionPool implements ConnectionPool {
     }
 
     @Override
-    public synchronized JRTConnection switchConnection() {
+    public synchronized JRTConnection switchConnection(Connection failingConnection) {
         List<JRTConnection> sources = getSources();
         if (sources.size() <= 1) return currentConnection;
 
-        List<JRTConnection> sourceCandidates = sources.stream()
-                                                    .filter(JRTConnection::isHealthy)
-                                                    .collect(Collectors.toList());
-        JRTConnection newConnection;
-        if (sourceCandidates.size() == 0) {
-            sourceCandidates = getSources();
-            sourceCandidates.remove(currentConnection);
-        }
-        newConnection = pickNewConnectionRandomly(sourceCandidates);
+        if ( ! currentConnection.equals(failingConnection)) return currentConnection;
+
+        return switchConnection();
+    }
+
+    /**
+     * Preconditions:
+     * 1. the current connection is unhealthy and should not be selected when switching
+     * 2. There is more than 1 source.
+     */
+    synchronized JRTConnection switchConnection() {
+        if (getSources().size() <= 1) throw new IllegalStateException("Cannot switch connection, not enough sources");
+
+        List<JRTConnection> sourceCandidates = getSources();
+        sourceCandidates.remove(currentConnection);
+        JRTConnection newConnection = pickNewConnectionRandomly(sourceCandidates);
         log.log(Level.INFO, () -> "Switching from " + currentConnection + " to " + newConnection);
         return currentConnection = newConnection;
     }
@@ -105,8 +111,7 @@ public class JRTConnectionPool implements ConnectionPool {
 
     @Override
     public void setError(Connection connection, int errorCode) {
-        connection.setError(errorCode);
-        switchConnection();
+        switchConnection(connection);
     }
 
     public JRTConnectionPool updateSources(List<String> addresses) {
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 4211345dff7..1b56e9290b2 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
@@ -277,7 +277,6 @@ public class JRTConfigRequesterTest {
         } catch (InterruptedException e) {
             e.printStackTrace();
         }
-        assertTrue(connection.getNumberOfFailovers() >= 1);
     }
 
     private JRTConfigSubscription<SimpletypesConfig> createSubscription(ConfigSubscriber subscriber, TimingValues timingValues) {
diff --git a/config/src/test/java/com/yahoo/vespa/config/JRTConnectionPoolTest.java b/config/src/test/java/com/yahoo/vespa/config/JRTConnectionPoolTest.java
index 08f9d0ad31d..bfe132c9660 100644
--- a/config/src/test/java/com/yahoo/vespa/config/JRTConnectionPoolTest.java
+++ b/config/src/test/java/com/yahoo/vespa/config/JRTConnectionPoolTest.java
@@ -29,18 +29,15 @@ import static org.junit.Assert.assertTrue;
 public class JRTConnectionPoolTest {
     private static final List<String> sources = new ArrayList<>((Arrays.asList("host0", "host1", "host2")));
 
-    /**
-     * Tests that hash-based selection through the list works.
-     */
     @Test
-    public void test_random_selection_of_sourceBasicHashBasedSelection() {
+    public void test_random_selection_of_source() {
         JRTConnectionPool sourcePool = new JRTConnectionPool(sources);
         assertEquals("host0,host1,host2",
                      sourcePool.getSources().stream().map(JRTConnection::getAddress).collect(Collectors.joining(",")));
 
         Map<String, Integer> sourceOccurrences = new HashMap<>();
         for (int i = 0; i < 1000; i++) {
-            final String address = sourcePool.switchConnection().getAddress();
+            String address = sourcePool.switchConnection().getAddress();
             if (sourceOccurrences.containsKey(address)) {
                 sourceOccurrences.put(address, sourceOccurrences.get(address) + 1);
             } else {
@@ -60,10 +57,7 @@ public class JRTConnectionPoolTest {
     public void testManySources() {
         Map<String, Integer> timesUsed = new LinkedHashMap<>();
 
-        List<String> twoSources = new ArrayList<>();
-
-        twoSources.add("host0");
-        twoSources.add("host1");
+        List<String> twoSources = List.of("host0", "host1");
         JRTConnectionPool sourcePool = new JRTConnectionPool(twoSources);
 
         int count = 1000;
@@ -99,10 +93,8 @@ public class JRTConnectionPoolTest {
      */
     @Test
     public void updateSources() {
-        List<String> twoSources = new ArrayList<>();
+        List<String> twoSources = List.of("host0", "host1");
 
-        twoSources.add("host0");
-        twoSources.add("host1");
         JRTConnectionPool sourcePool = new JRTConnectionPool(twoSources);
 
         ConfigSourceSet sourcesBefore = sourcePool.getSourceSet();
@@ -138,36 +130,38 @@ public class JRTConnectionPoolTest {
 
     @Test
     public void testFailingSources() {
-        List<String> sources = new ArrayList<>();
+        List<String> sources = List.of("host0", "host1", "host2");
+        JRTConnectionPool connectionPool = new JRTConnectionPool(sources);
 
-        sources.add("host0");
-        sources.add("host1");
-        sources.add("host2");
-        JRTConnectionPool sourcePool = new JRTConnectionPool(sources);
+        Connection firstConnection = connectionPool.getCurrent();
 
-        Connection firstConnection = sourcePool.getCurrent();
+        // Should change connection, not getting first connection as new
+        JRTConnection secondConnection = failAndGetNewConnection(connectionPool, firstConnection);
+        assertNotEquals(firstConnection, secondConnection);
 
-        // Should change connection away from first connection
-        sourcePool.setError(firstConnection, 123);
-        JRTConnection secondConnection = sourcePool.getCurrent();
-        assertNotEquals(secondConnection, firstConnection);
+        // Should change connection, not getting second connection as new
+        JRTConnection thirdConnection = failAndGetNewConnection(connectionPool, secondConnection);
+        // Fail a few more times with old connection, as will happen when there are multiple subscribers
+        // Connection should not change
+        assertEquals(thirdConnection, failAndGetNewConnection(connectionPool, secondConnection));
+        assertEquals(thirdConnection, failAndGetNewConnection(connectionPool, secondConnection));
+        assertEquals(thirdConnection, failAndGetNewConnection(connectionPool, secondConnection));
+        assertNotEquals(secondConnection, thirdConnection);
 
-        // Should change connection away from first AND second connection
-        sourcePool.setError(secondConnection, 123);
-        JRTConnection thirdConnection = sourcePool.getCurrent();
-        assertNotEquals(sourcePool.getCurrent(), firstConnection);
-        assertNotEquals(sourcePool.getCurrent(), secondConnection);
+        // Should change connection, not getting third connection as new
+        JRTConnection currentConnection = failAndGetNewConnection(connectionPool, thirdConnection);
+        assertNotEquals(thirdConnection, currentConnection);
 
-        // Should change connection away from third connection
-        sourcePool.setError(thirdConnection, 123);
-        JRTConnection currentConnection = sourcePool.getCurrent();
-        assertNotEquals(sourcePool.getCurrent(), thirdConnection);
+        // Should change connection, not getting current connection as new
+        JRTConnection currentConnection2 = failAndGetNewConnection(connectionPool, currentConnection);
+        assertNotEquals(currentConnection, currentConnection2);
 
-        // Should change connection from current connection
-        sourcePool.setError(thirdConnection, 123);
-        assertNotEquals(sourcePool.getCurrent(), currentConnection);
+        connectionPool.close();
+    }
 
-        sourcePool.close();
+    private JRTConnection failAndGetNewConnection(JRTConnectionPool connectionPool, Connection failingConnection) {
+        connectionPool.setError(failingConnection, 123);
+        return connectionPool.getCurrent();
     }
 
 }
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDistributionUtil.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDistributionUtil.java
index 8dc9272b1b7..33cd425d6aa 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDistributionUtil.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileDistributionUtil.java
@@ -79,7 +79,7 @@ public class FileDistributionUtil {
         public Connection getCurrent() { return null; }
 
         @Override
-        public Connection switchConnection() { return null; }
+        public Connection switchConnection(Connection connection) { return null; }
 
         @Override
         public int getSize() { return 0; }
diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
index 05fbd457a0d..da9d4ceab88 100644
--- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
+++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReferenceDownloader.java
@@ -127,7 +127,7 @@ public class FileReferenceDownloader {
                 return true;
             } else {
                 log.log(logLevel, "File reference '" + fileReference + "' not found at " + connection.getAddress());
-                connectionPool.switchConnection();
+                connectionPool.switchConnection(connection);
                 return false;
             }
         } else {
diff --git a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
index 1344b7afbb3..61575b650ce 100644
--- a/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
+++ b/filedistribution/src/test/java/com/yahoo/vespa/filedistribution/FileDownloaderTest.java
@@ -314,14 +314,6 @@ public class FileDownloaderTest {
         }
 
         @Override
-        public void setError(int errorCode) {
-        }
-
-        @Override
-        public void setSuccess() {
-        }
-
-        @Override
         public String getAddress() {
             return null;
         }
@@ -331,9 +323,7 @@ public class FileDownloaderTest {
         }
 
         @Override
-        public void setError(Connection connection, int errorCode) {
-            connection.setError(errorCode);
-        }
+        public void setError(Connection connection, int errorCode) { }
 
         @Override
         public Connection getCurrent() {
@@ -341,7 +331,7 @@ public class FileDownloaderTest {
         }
 
         @Override
-        public Connection switchConnection() {
+        public Connection switchConnection(Connection connection) {
             return this;
         }