Remove ReadOnlyStatusService

8 files changed, 32 insertions, 92 deletions

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<ClusterApi> clusterInOrder;
     private final Map<HostName, HostStatus> hostStatusMap;
 
@@ -41,6 +42,7 @@ public class ApplicationApiImpl implements ApplicationApi {
         this.applicationInstance = nodeGroup.getApplication();
         this.nodeGroup = nodeGroup;
         this.hostStatusService = hostStatusService;
+        this.statusService = statusService;
         Collection<HostName> hosts = getHostsUsedByApplicationInstance(applicationInstance);
         Collection<HostName> 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<ApplicationInstanceReference> 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<ApplicationInstanceReference, Set<HostName>> 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
@@ -56,21 +56,6 @@ public class ZookeeperStatusService implements StatusService {
     }
 
     @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<ApplicationInstanceReference> getAllSuspendedApplications() {
         try {
             Set<ApplicationInstanceReference> resultSet = new HashSet<>();
@@ -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();
@@ -291,16 +271,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 {
                 lock.close();
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));
     }