Code review based improvements.

10 files changed, 61 insertions, 42 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
index c0d9751145d..147f7cdab9f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdmin.java
@@ -11,11 +11,13 @@ import java.util.Set;
  */
 public interface NodeAdmin {
 
-     void setState(final List<ContainerNodeSpec> containersToRun);
+    void setState(final List<ContainerNodeSpec> containersToRun);
 
-     boolean setFreezeAndCheckIfAllFrozen(boolean freeze);
+    boolean freezeAndCheckIfAllFrozen();
 
-     Set<HostName> getListOfHosts();
+    void unfreeze();
 
-     String debugInfo();
+    Set<HostName> getListOfHosts();
+
+    String debugInfo();
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
index 196fb9ac8cb..ce59c19ec5a 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminImpl.java
@@ -56,18 +56,24 @@ public class NodeAdminImpl implements NodeAdmin {
         garbageCollectDockerImages(containersToRun);
     }
 
-    public boolean setFreezeAndCheckIfAllFrozen(boolean freeze) {
+    public boolean freezeAndCheckIfAllFrozen() {
         for (NodeAgent nodeAgent : nodeAgents.values()) {
-            nodeAgent.execute(freeze ? NodeAgent.Command.FREEZE : NodeAgent.Command.UNFREEZE);
+            nodeAgent.execute(NodeAgent.Command.FREEZE);
         }
         for (NodeAgent nodeAgent : nodeAgents.values()) {
-            if (freeze && nodeAgent.getState() != NodeAgent.State.FROZEN) {
+            if (nodeAgent.getState() != NodeAgent.State.FROZEN) {
                 return false;
             }
         }
         return true;
     }
 
+    public void unfreeze() {
+        for (NodeAgent nodeAgent : nodeAgents.values()) {
+            nodeAgent.execute(NodeAgent.Command.UNFREEZE);
+        }
+    }
+
     public Set<HostName> getListOfHosts() {
         return nodeAgents.keySet();
     }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
index c64534c059a..97bcdfa724a 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdater.java
@@ -80,13 +80,11 @@ public class NodeAdminStateUpdater extends AbstractComponent {
             isRunningUpdates = wantedState == RESUMED;
         }
         if (wantedState == SUSPENDED) {
-            if (! nodeAdmin.setFreezeAndCheckIfAllFrozen(true)) {
+            if (! nodeAdmin.freezeAndCheckIfAllFrozen()) {
                 return Optional.of("Not all node agents are frozen.");
             }
         } else {
-            if (nodeAdmin.setFreezeAndCheckIfAllFrozen(false)) {
-                return Optional.of("Not all node agents are unfrozen.");
-            }
+            nodeAdmin.unfreeze();
         }
 
         List<String> hosts = new ArrayList<>();
@@ -103,7 +101,6 @@ public class NodeAdminStateUpdater extends AbstractComponent {
                 log.log(Level.FINE, "Is frozen, skipping");
                 return;
             }
-            // TODO: should the result from the config server contain both active and inactive?
             final List<ContainerNodeSpec> containersToRun;
             try {
                 containersToRun = nodeRepository.getContainersToRun();
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
index 29e38391c65..d4af21900c3 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/NodeAgentImpl.java
@@ -390,29 +390,38 @@ public class NodeAgentImpl implements NodeAgent {
         monitor.notifyAll();
     }
 
-    private void maintainWantedState()  {
-        while (true) {
+    private void blockUntilNotWaitingOrFrozen() {
             try {
                 synchronized (monitor) {
-                    switch (wantedState) {
-                        case WAITING:
-                            state = State.WAITING;
-                            monitor.wait();
-                            continue;
-                        case WORKING:
-                            state = State.WORKING;
-                            break;
-                        case FROZEN:
-                            state = State.FROZEN;
-                            monitor.wait();
-                            break;
-                        case TERMINATED:
-                            return;
+                    while (wantedState == State.WAITING || wantedState == State.FROZEN) {
+                        state = wantedState;
+                        monitor.wait();
+                        continue;
                     }
                 }
             } catch (InterruptedException e) {
                 throw new RuntimeException(e);
             }
+    }
+
+    private void maintainWantedState()  {
+        while (true) {
+            blockUntilNotWaitingOrFrozen();
+            synchronized (monitor) {
+                switch (wantedState) {
+                    case WAITING:
+                        state = State.WAITING;
+                        continue;
+                    case WORKING:
+                        state = State.WORKING;
+                        break;
+                    case FROZEN:
+                        state = State.FROZEN;
+                        continue;
+                    case TERMINATED:
+                        return;
+                }
+            }
             // This is WORKING state.
             try {
                 final ContainerNodeSpec nodeSpec = nodeRepository.getContainerNodeSpec(hostname)
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
index 921fe47ed9f..06fb9f63148 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/restapi/RestApiHandler.java
@@ -24,7 +24,7 @@ import static com.yahoo.jdisc.http.HttpRequest.Method.PUT;
  *
  * There is one debug call: /info
  *
- * @autor dybis
+ * @author dybis
  */
 public class RestApiHandler extends LoggingRequestHandler{
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
index ba74bff07bb..f9d9ffdf0be 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/NodeAdminStateUpdaterTest.java
@@ -62,7 +62,6 @@ public class NodeAdminStateUpdaterTest {
         assertThat(accumulatedArgumentList.get(0), is(createSample()));
         Thread.sleep(2);
         assertThat(accumulatedArgumentList.size(), is(numberOfElements));
-        when(nodeAdmin.setFreezeAndCheckIfAllFrozen(false)).thenReturn(false);
         assertThat(refresher.setResumeStateAndCheckIfResumed(NodeAdminStateUpdater.State.RESUMED),
                 is(Optional.empty()));
         while (accumulatedArgumentList.size() == numberOfElements) {
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
index 942ea4423d1..4043195ea5e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeAdminMock.java
@@ -12,7 +12,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
 /**
  * Mock with some simple logic
  *
- * @autor dybis
+ * @author dybis
  */
 public class NodeAdminMock implements NodeAdmin {
 
@@ -35,13 +35,19 @@ public class NodeAdminMock implements NodeAdmin {
     }
 
     @Override
-    public boolean setFreezeAndCheckIfAllFrozen(boolean freeze) {
-        info.append(" Freeze called with " + freeze + " while in state " + frozen.get());
-        freezeSetState = freeze;
+    public boolean freezeAndCheckIfAllFrozen() {
+        info.append(" Freeze called while in state " + frozen.get());
+        freezeSetState = true;
         return frozen.get();
     }
 
     @Override
+    public void unfreeze() {
+        info.append(" Unfreeze called while in state " + frozen.get());
+        freezeSetState = false;
+    }
+
+    @Override
     public Set<HostName> getListOfHosts() {
         synchronized (monitor) {
             return hostnames;
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
index a5233a07ce0..1d4bb297e49 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/NodeRepoMock.java
@@ -12,7 +12,7 @@ import java.util.Optional;
 
 /**
  * Mock with some simple logic
- * @autor dybis
+ * @author dybis
  */
 public class NodeRepoMock implements NodeRepository {
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
index cdd1dc10bf3..b9abce88f38 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/OrchestratorMock.java
@@ -8,7 +8,7 @@ import java.util.Optional;
 
 /**
  * Mock with some simple logic
- * @autor dybis
+ * @author dybis
  */
 public class OrchestratorMock implements Orchestrator {
 
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
index 2e0359f1106..b704e4624b7 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/RunInContainerTest.java
@@ -55,10 +55,10 @@ public class RunInContainerTest {
         container = JDisc.fromServicesXml(createServiceXml(port), Networking.enable);
     }
 
-    private boolean doPutCall(boolean resume) throws IOException {
+    private boolean doPutCall(String command) throws IOException {
         HttpClient httpclient = HttpClientBuilder.create().build();
         HttpHost target = new HttpHost("localhost", port, "http");
-        HttpPut getRequest = new HttpPut(resume ? "/rest/resume" : "/rest/suspend");
+        HttpPut getRequest = new HttpPut("/rest/" + command);
         HttpResponse httpResponse = httpclient.execute(target, getRequest);
         return httpResponse.getStatusLine().getStatusCode() == 200;
     }
@@ -105,11 +105,11 @@ public class RunInContainerTest {
     @Test
     public void testGetContainersToRunAPi() throws IOException, InterruptedException {
         waitForJdiscContainerToServe();
-        assertThat(doPutCall(true), is(true));
-        assertThat(doPutCall(false), is(false));
+        assertThat(doPutCall("resume"), is(true));
+        assertThat(doPutCall("suspend"), is(false));
         assertThat(doGetInfoCall(), is("{ \"jsonMessage\":\"isRunningUpdates is false. NodeAdmin:  " +
-                "Freeze called with false while in state false " +
-                "Freeze called with true while in state false\"}"));
+                "Unfreeze called while in state false " +
+                "Freeze called while in state false\"}"));
     }