From 9a8780786ac09415dbcfec5d16b0585e3b4c533a Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 8 Feb 2019 11:55:01 +0100 Subject: Remove InMemoryStatusService, and clean up curator usage in ZookeeperStatusServiceTest --- .../orchestrator/status/InMemoryStatusService.java | 122 --------------------- .../vespa/orchestrator/OrchestratorImplTest.java | 5 +- .../status/ZookeeperStatusServiceTest.java | 73 ++++++------ 3 files changed, 35 insertions(+), 165 deletions(-) delete mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/InMemoryStatusService.java 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 hostServiceStatus = new HashMap<>(); - private final Set applicationStatus = new HashSet<>(); - private final LockService 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 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 { - - private final Map 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 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 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 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 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()); } } -- cgit v1.2.3