From d6b759ae7507ed6eedfd7ee99ef3147efacf6c9f Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Fri, 8 Feb 2019 17:12:52 +0100 Subject: Remove ReadOnlyStatusService --- .../yahoo/vespa/orchestrator/OrchestratorImpl.java | 12 +++---- .../yahoo/vespa/orchestrator/OrchestratorUtil.java | 2 -- .../orchestrator/model/ApplicationApiImpl.java | 4 ++- .../orchestrator/status/MutableStatusRegistry.java | 2 +- .../status/ReadOnlyStatusRegistry.java | 25 -------------- .../vespa/orchestrator/status/StatusService.java | 23 ++++++------- .../status/ZookeeperStatusService.java | 38 +++------------------- .../status/ZookeeperStatusServiceTest.java | 18 ++++------ 8 files changed, 32 insertions(+), 92 deletions(-) delete mode 100644 orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java index 5d5d6df0fb3..ff0299bdc17 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorImpl.java @@ -143,13 +143,13 @@ public class OrchestratorImpl implements Orchestrator { OrchestratorContext context = OrchestratorContext.createContextForSingleAppOp(clock); try (MutableStatusRegistry statusRegistry = statusService .lockApplicationInstance_forCurrentThreadOnly(context, appInstance.reference())) { - final HostStatus currentHostState = statusRegistry.getHostStatus(hostName); + HostStatus currentHostState = statusService.getHostStatus(appInstance.reference(), hostName); if (HostStatus.NO_REMARKS == currentHostState) { return; } - ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(appInstance.reference()).getApplicationInstanceStatus(); + ApplicationInstanceStatus appStatus = statusService.getApplicationInstanceStatus(appInstance.reference()); if (appStatus == ApplicationInstanceStatus.NO_REMARKS) { policy.releaseSuspensionGrant(context.createSubcontextWithinLock(), appInstance, hostName, statusRegistry, statusService); } @@ -192,7 +192,7 @@ public class OrchestratorImpl implements Orchestrator { try (MutableStatusRegistry hostStatusRegistry = statusService.lockApplicationInstance_forCurrentThreadOnly(context, applicationReference)) { - ApplicationInstanceStatus appStatus = statusService.forApplicationInstance(applicationReference).getApplicationInstanceStatus(); + ApplicationInstanceStatus appStatus = statusService.getApplicationInstanceStatus(applicationReference); if (appStatus == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { return; } @@ -208,7 +208,7 @@ public class OrchestratorImpl implements Orchestrator { @Override public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationId appId) throws ApplicationIdNotFoundException { ApplicationInstanceReference appRef = OrchestratorUtil.toApplicationInstanceReference(appId,instanceLookupService); - return statusService.forApplicationInstance(appRef).getApplicationInstanceStatus(); + return statusService.getApplicationInstanceStatus(appRef); } @Override @@ -317,7 +317,7 @@ public class OrchestratorImpl implements Orchestrator { } private HostStatus getNodeStatus(ApplicationInstanceReference applicationRef, HostName hostName) { - return statusService.forApplicationInstance(applicationRef).getHostStatus(hostName); + return statusService.getHostStatus(applicationRef, hostName); } private void setApplicationStatus(ApplicationId appId, ApplicationInstanceStatus status) @@ -328,7 +328,7 @@ public class OrchestratorImpl implements Orchestrator { statusService.lockApplicationInstance_forCurrentThreadOnly(context, appRef)) { // Short-circuit if already in wanted state - if (status == statusRegistry.getApplicationInstanceStatus()) return; + if (status == statusService.getApplicationInstanceStatus(appRef)) return; // Set content clusters for this application in maintenance on suspend if (status == ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN) { diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java index de8f7e9f2bc..a4a6baf1669 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/OrchestratorUtil.java @@ -12,8 +12,6 @@ import com.yahoo.vespa.applicationmodel.HostName; import com.yahoo.vespa.applicationmodel.ServiceCluster; import com.yahoo.vespa.applicationmodel.ServiceInstance; import com.yahoo.vespa.applicationmodel.TenantId; -import com.yahoo.vespa.orchestrator.status.HostStatus; -import com.yahoo.vespa.orchestrator.status.ReadOnlyStatusRegistry; import java.util.Collection; import java.util.List; diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java index 1d74fb337d9..3343849dad5 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/model/ApplicationApiImpl.java @@ -31,6 +31,7 @@ public class ApplicationApiImpl implements ApplicationApi { private final ApplicationInstance applicationInstance; private final NodeGroup nodeGroup; private final MutableStatusRegistry hostStatusService; + private final StatusService statusService; private final List clusterInOrder; private final Map hostStatusMap; @@ -41,6 +42,7 @@ public class ApplicationApiImpl implements ApplicationApi { this.applicationInstance = nodeGroup.getApplication(); this.nodeGroup = nodeGroup; this.hostStatusService = hostStatusService; + this.statusService = statusService; Collection hosts = getHostsUsedByApplicationInstance(applicationInstance); Collection suspendedHosts = statusService.getSuspendedHostsByApplication().apply(nodeGroup.getApplicationReference()); this.hostStatusMap = hosts.stream().collect(Collectors.toMap(Function.identity(), @@ -96,7 +98,7 @@ public class ApplicationApiImpl implements ApplicationApi { @Override public ApplicationInstanceStatus getApplicationStatus() { - return hostStatusService.getApplicationInstanceStatus(); + return statusService.getApplicationInstanceStatus(nodeGroup.getApplicationReference()); } @Override diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java index 18378092a4e..7e0fe5241a0 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/MutableStatusRegistry.java @@ -10,7 +10,7 @@ import com.yahoo.vespa.applicationmodel.HostName; * @author Tony Vaagenes * @author bakksjo */ -public interface MutableStatusRegistry extends ReadOnlyStatusRegistry, AutoCloseable { +public interface MutableStatusRegistry extends AutoCloseable { /** * Sets the state for the given host. diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java deleted file mode 100644 index 09300ef18a8..00000000000 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ReadOnlyStatusRegistry.java +++ /dev/null @@ -1,25 +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.HostName; - -/** - * Read-only view of statuses for the application instance and its hosts. - * - * @author oyving - * @author Tony Vaagenes - * @author bakksjo - */ -public interface ReadOnlyStatusRegistry { - - /** - * Gets the current state for the given host. - */ - HostStatus getHostStatus(HostName hostName); - - /** - * Gets the current status for the application instance. - */ - ApplicationInstanceStatus getApplicationInstanceStatus(); - -} diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java index 37f814ef03a..9f91e08d344 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/StatusService.java @@ -18,16 +18,6 @@ import java.util.function.Function; * @author smorgrav */ public interface StatusService { - /** - * Returns a readable host status registry for the given application instance. No locking is involved, - * so this call will never block. However, since it is possible that mutations are going on simultaneously - * with accessing this registry, the view obtained through the returned registry must be considered to be - * possibly inconsistent snapshot values. It is not recommended that this method is used for anything other - * than monitoring, logging, debugging, etc. It should never be used for multi-step operations (e.g. - * read-then-write) where consistency is required. For those cases, use - * {@link #lockApplicationInstance_forCurrentThreadOnly(OrchestratorContext, ApplicationInstanceReference)}. - */ - ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference); /** * Returns a mutable host status registry for a locked application instance. All operations performed on @@ -66,11 +56,22 @@ public interface StatusService { Set getAllSuspendedApplications(); /** - * Returns a not necessarily consistent mapping from applications to their set of suspended hosts. + * Returns a fresh, but not necessarily consistent mapping from applications to their set of suspended hosts. * * If the lock for an application is held when this mapping is acquired, new sets returned for that application * are consistent and up to date for as long as the lock is held. (The sets themselves don't reflect changes.) */ Function> getSuspendedHostsByApplication(); + /** + * Returns the status of the given application. This is consistent if its lock is held. + */ + ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationInstanceReference application); + + + /** + * Returns the status of the given host, for the given application. This is consistent if the application's lock is held. + */ + HostStatus getHostStatus(ApplicationInstanceReference application, HostName host); + } diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java index 0ed0d6ad31a..5a16f4411b5 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java @@ -55,21 +55,6 @@ public class ZookeeperStatusService implements StatusService { this.hostsDown = new ConcurrentHashMap<>(); } - @Override - public ReadOnlyStatusRegistry forApplicationInstance(ApplicationInstanceReference applicationInstanceReference) { - return new ReadOnlyStatusRegistry() { - @Override - public HostStatus getHostStatus(HostName hostName) { - return getInternalHostStatus(applicationInstanceReference, hostName); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return getInternalApplicationInstanceStatus(applicationInstanceReference); - } - }; - } - @Override public Set getAllSuspendedApplications() { try { @@ -178,7 +163,8 @@ public class ZookeeperStatusService implements StatusService { } } - private HostStatus getInternalHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName) { + @Override + public HostStatus getHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName) { return getValidCache().computeIfAbsent(applicationInstanceReference, this::hostsDownFor) .contains(hostName) ? HostStatus.ALLOWED_TO_BE_DOWN : HostStatus.NO_REMARKS; } @@ -207,8 +193,8 @@ public class ZookeeperStatusService implements StatusService { } } - /** Common implementation for the two internal classes that sets ApplicationInstanceStatus. */ - private ApplicationInstanceStatus getInternalApplicationInstanceStatus(ApplicationInstanceReference applicationInstanceReference) { + @Override + public ApplicationInstanceStatus getApplicationInstanceStatus(ApplicationInstanceReference applicationInstanceReference) { try { Stat statOrNull = curator.framework().checkExists().forPath( applicationInstanceSuspendedPath(applicationInstanceReference)); @@ -219,12 +205,6 @@ public class ZookeeperStatusService implements StatusService { } } - private HostStatus getHostStatusWithLock( - final ApplicationInstanceReference applicationInstanceReference, - final HostName hostName) { - return getInternalHostStatus(applicationInstanceReference, hostName); - } - private static String applicationInstancePath(ApplicationInstanceReference applicationInstanceReference) { return HOST_STATUS_BASE_PATH + '/' + applicationInstanceReference.tenantId() + ":" + applicationInstanceReference.applicationInstanceId(); @@ -290,16 +270,6 @@ public class ZookeeperStatusService implements StatusService { } } - @Override - public HostStatus getHostStatus(final HostName hostName) { - return getHostStatusWithLock(applicationInstanceReference, hostName); - } - - @Override - public ApplicationInstanceStatus getApplicationInstanceStatus() { - return getInternalApplicationInstanceStatus(applicationInstanceReference); - } - @Override public void close() { try { 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 57fceb4c24b..809ca7e5c97 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 @@ -75,8 +75,7 @@ public class ZookeeperStatusServiceTest { @Test public void host_state_for_unknown_hosts_is_no_remarks() { assertThat( - zookeeperStatusService.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getHostStatus(TestIds.HOST_NAME1), + zookeeperStatusService.getHostStatus(TestIds.APPLICATION_INSTANCE_REFERENCE, TestIds.HOST_NAME1), is(HostStatus.NO_REMARKS)); } @@ -92,8 +91,7 @@ public class ZookeeperStatusServiceTest { TestIds.HOST_NAME1, hostStatus); - assertThat(statusRegistry.getHostStatus( - TestIds.HOST_NAME1), + assertThat(zookeeperStatusService.getHostStatus(TestIds.APPLICATION_INSTANCE_REFERENCE, TestIds.HOST_NAME1), is(hostStatus)); } } @@ -143,8 +141,7 @@ public class ZookeeperStatusServiceTest { killSession(curator.framework(), testingServer); //Throws SessionFailedException if the SessionFailRetryLoop has not been closed. - zookeeperStatusService2.forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getHostStatus(TestIds.HOST_NAME1); + zookeeperStatusService2.getHostStatus(TestIds.APPLICATION_INSTANCE_REFERENCE, TestIds.HOST_NAME1); }); assertThat(resultOfZkOperationAfterLockFailure, notHoldsException()); @@ -202,8 +199,7 @@ public class ZookeeperStatusServiceTest { // Initial state is NO_REMARK assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.NO_REMARKS)); // Suspend @@ -214,8 +210,7 @@ public class ZookeeperStatusServiceTest { assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.ALLOWED_TO_BE_DOWN)); // Resume @@ -226,8 +221,7 @@ public class ZookeeperStatusServiceTest { assertThat( zookeeperStatusService - .forApplicationInstance(TestIds.APPLICATION_INSTANCE_REFERENCE) - .getApplicationInstanceStatus(), + .getApplicationInstanceStatus(TestIds.APPLICATION_INSTANCE_REFERENCE), is(ApplicationInstanceStatus.NO_REMARKS)); } -- cgit v1.2.3