Remove InMemoryStatusService, and clean up curator usage in ZookeeperStatusServiceTest

3 files changed, 35 insertions, 165 deletions

diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java
deleted file mode 100644
index 41044bae782..00000000000
--- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java
+++ /dev/null
@@ -1,122 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.orchestrator.status;
-
-import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference;
-import com.yahoo.vespa.applicationmodel.HostName;
-import com.yahoo.vespa.orchestrator.OrchestratorContext;
-
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
-
-/**
- * Implementation of the StatusService interface for testing.
- *
- * @author oyving
- */
-public class InMemoryStatusService implements StatusService {
-
-    private final Map<HostName, HostStatus> hostServiceStatus = new HashMap<>();
-    private final Set<ApplicationInstanceReference> applicationStatus = new HashSet<>();
-    private final LockService<ApplicationInstanceReference> instanceLockService = new LockService<>();
-
-    private void setHostStatus(HostName hostName, HostStatus status) {
-        hostServiceStatus.put(hostName, status);
-    }
-
-    @Override
-    public ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference) {
-        return new ReadOnlyStatusRegistry() {
-            @Override
-            public HostStatus getHostStatus(HostName hostName) {
-                return hostServiceStatus.getOrDefault(hostName, HostStatus.NO_REMARKS);
-            }
-
-            @Override
-            public ApplicationInstanceStatus getApplicationInstanceStatus() {
-                return applicationStatus.contains(applicationInstanceReference) ?
-                        ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN : ApplicationInstanceStatus.NO_REMARKS;
-            }
-        };
-    }
-
-
-    @Override
-    public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly(
-            OrchestratorContext context,
-            ApplicationInstanceReference applicationInstanceReference) {
-        return new InMemoryMutableStatusRegistry(applicationInstanceReference);
-    }
-
-    @Override
-    public Set<ApplicationInstanceReference> getAllSuspendedApplications() {
-        return applicationStatus;
-    }
-
-    private class InMemoryMutableStatusRegistry implements MutableStatusRegistry {
-
-        private final Lock lockHandle;
-        private final ApplicationInstanceReference ref;
-
-        private InMemoryMutableStatusRegistry(ApplicationInstanceReference ref) {
-            this.ref = ref;
-
-            this.lockHandle = instanceLockService.get(ref);
-            this.lockHandle.lock();
-        }
-
-        @Override
-        public void setHostState(HostName hostName, HostStatus status) {
-            setHostStatus(hostName, status);
-        }
-
-        @Override
-        public void setApplicationInstanceStatus(ApplicationInstanceStatus applicationInstanceStatus) {
-            if (applicationInstanceStatus == ApplicationInstanceStatus.NO_REMARKS) {
-                applicationStatus.remove(ref);
-            } else {
-                applicationStatus.add(ref);
-            }
-        }
-
-        @Override
-        public HostStatus getHostStatus(HostName hostName) {
-            return hostServiceStatus.getOrDefault(hostName, HostStatus.NO_REMARKS);
-        }
-
-        @Override
-        public ApplicationInstanceStatus getApplicationInstanceStatus() {
-            return applicationStatus.contains(ref) ? ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN :
-            ApplicationInstanceStatus.NO_REMARKS;
-        }
-
-        @Override
-        public void close() {
-            lockHandle.unlock();
-        }
-    }
-
-    private static class LockService<T> {
-
-        private final Map<T, Lock> locks;
-
-        public LockService() {
-            this.locks = new HashMap<>();
-        }
-
-        public Lock get(T lockId) {
-            synchronized (this) {
-                Lock lock = locks.computeIfAbsent(
-                    lockId,
-                    id -> new ReentrantLock()
-                );
-
-                return lock;
-            }
-        }
-    }
-
-}
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
index cc6b9a7dbf7..856a1ffb000 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java
@@ -13,6 +13,7 @@ import com.yahoo.vespa.applicationmodel.ServiceInstance;
 import com.yahoo.vespa.applicationmodel.ServiceStatus;
 import com.yahoo.vespa.applicationmodel.ServiceType;
 import com.yahoo.vespa.applicationmodel.TenantId;
+import com.yahoo.vespa.curator.mock.MockCurator;
 import com.yahoo.vespa.orchestrator.config.OrchestratorConfig;
 import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactory;
 import com.yahoo.vespa.orchestrator.controller.ClusterControllerClientFactoryMock;
@@ -20,9 +21,9 @@ import com.yahoo.vespa.orchestrator.model.NodeGroup;
 import com.yahoo.vespa.orchestrator.policy.BatchHostStateChangeDeniedException;
 import com.yahoo.vespa.orchestrator.policy.HostStateChangeDeniedException;
 import com.yahoo.vespa.orchestrator.status.HostStatus;
-import com.yahoo.vespa.orchestrator.status.InMemoryStatusService;
 import com.yahoo.vespa.orchestrator.status.ReadOnlyStatusRegistry;
 import com.yahoo.vespa.orchestrator.status.StatusService;
+import com.yahoo.vespa.orchestrator.status.ZookeeperStatusService;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -79,7 +80,7 @@ public class OrchestratorImplTest {
         clustercontroller = new ClusterControllerClientFactoryMock();
         orchestrator = new OrchestratorImpl(
                 clustercontroller,
-                new InMemoryStatusService(),
+                new ZookeeperStatusService(new MockCurator()),
                 new OrchestratorConfig(new OrchestratorConfig.Builder()),
                 new DummyInstanceLookupService());
 
diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java
index 818a184024b..57fceb4c24b 100644
--- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java
+++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java
@@ -7,7 +7,6 @@ import com.yahoo.vespa.curator.Curator;
 import com.yahoo.vespa.orchestrator.OrchestratorContext;
 import com.yahoo.vespa.orchestrator.TestIds;
 import org.apache.commons.lang.exception.ExceptionUtils;
-import org.apache.curator.SessionFailRetryLoop.SessionFailedException;
 import org.apache.curator.framework.CuratorFramework;
 import org.apache.curator.test.KillSession;
 import org.apache.curator.test.TestingServer;
@@ -56,10 +55,6 @@ public class ZookeeperStatusServiceTest {
         when(context.isProbe()).thenReturn(false);
     }
 
-    private static Curator createConnectedCuratorFramework(TestingServer server) throws InterruptedException {
-        return createConnectedCurator(server);
-    }
-
     private static Curator createConnectedCurator(TestingServer server) throws InterruptedException {
         Curator curator = Curator.create(server.getConnectString());
         curator.framework().blockUntilConnected(1, TimeUnit.MINUTES);
@@ -107,56 +102,52 @@ public class ZookeeperStatusServiceTest {
 
     @Test
     public void locks_are_exclusive() throws Exception {
-        try (Curator curator = createConnectedCuratorFramework(testingServer)) {
-            ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator);
+        ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator);
 
-            final CompletableFuture<Void> lockedSuccessfullyFuture;
-            try (MutableStatusRegistry statusRegistry = zookeeperStatusService
-                    .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) {
-
-                lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> {
-                    try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2
-                            .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE))
-                    {
-                    }
-                });
+        final CompletableFuture<Void> lockedSuccessfullyFuture;
+        try (MutableStatusRegistry statusRegistry = zookeeperStatusService
+                .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) {
 
-                try {
-                    lockedSuccessfullyFuture.get(3, TimeUnit.SECONDS);
-                    fail("Both zookeeper host status services locked simultaneously for the same application instance");
-                } catch (TimeoutException ignored) {
+            lockedSuccessfullyFuture = CompletableFuture.runAsync(() -> {
+                try (MutableStatusRegistry statusRegistry2 = zookeeperStatusService2
+                        .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE))
+                {
                 }
-            }
+            });
 
-            lockedSuccessfullyFuture.get(1, TimeUnit.MINUTES);
+            try {
+                lockedSuccessfullyFuture.get(3, TimeUnit.SECONDS);
+                fail("Both zookeeper host status services locked simultaneously for the same application instance");
+            } catch (TimeoutException ignored) {
+            }
         }
+
+        lockedSuccessfullyFuture.get(1, TimeUnit.MINUTES);
     }
 
     @Test
     public void failing_to_get_lock_closes_SessionFailRetryLoop() throws Exception {
-        try (Curator curator = createConnectedCuratorFramework(testingServer)) {
-            ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator);
+        ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator);
 
-            try (MutableStatusRegistry statusRegistry = zookeeperStatusService
-                    .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) {
+        try (MutableStatusRegistry statusRegistry = zookeeperStatusService
+                .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) {
 
-                //must run in separate thread, since having 2 locks in the same thread fails
-                CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> {
-                    try {
-                        zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE);
-                        fail("Both zookeeper host status services locked simultaneously for the same application instance");
-                    } catch (RuntimeException e) {
-                    }
+            //must run in separate thread, since having 2 locks in the same thread fails
+            CompletableFuture<Void> resultOfZkOperationAfterLockFailure = CompletableFuture.runAsync(() -> {
+                try {
+                    zookeeperStatusService2.lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE);
+                    fail("Both zookeeper host status services locked simultaneously for the same application instance");
+                } catch (RuntimeException e) {
+                }
 
-                    killSession(curator.framework(), testingServer);
+                killSession(curator.framework(), testingServer);
 
-                    //Throws SessionFailedException if the SessionFailRetryLoop has not been closed.
-                    zookeeperStatusService2.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE)
-                            .getHostStatus(TestIds.HOST_NAME1);
-                });
+                //Throws SessionFailedException if the SessionFailRetryLoop has not been closed.
+                zookeeperStatusService2.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE)
+                                       .getHostStatus(TestIds.HOST_NAME1);
+            });
 
-                assertThat(resultOfZkOperationAfterLockFailure, notHoldsException());
-            }
+            assertThat(resultOfZkOperationAfterLockFailure, notHoldsException());
         }
     }