Merge pull request #1867 from yahoo/hmusum/orchestrator-client-minor-refactoring

Minor refactoring of orchestrator client

2 files changed, 30 insertions, 31 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
index b91c2411d95..33a2a8d81a5 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/orchestrator/OrchestratorImpl.java
@@ -14,7 +14,6 @@ import com.yahoo.vespa.orchestrator.restapi.wire.BatchOperationResult;
 import com.yahoo.vespa.orchestrator.restapi.wire.UpdateHostResponse;
 import java.util.List;
 import java.util.Optional;
-import java.util.Set;
 
 /**
  * @author stiankri
@@ -33,36 +32,26 @@ public class OrchestratorImpl implements Orchestrator {
 
     private final ConfigServerHttpRequestExecutor requestExecutor;
 
-    // For testing
-    OrchestratorImpl(ConfigServerHttpRequestExecutor requestExecutor) {
+    public OrchestratorImpl(ConfigServerHttpRequestExecutor requestExecutor) {
         this.requestExecutor = requestExecutor;
     }
 
-    public OrchestratorImpl(Set<String> configServerHosts) {
-        if (configServerHosts.isEmpty()) {
-            throw new IllegalStateException("Environment setting for config servers missing or empty.");
-        }
-        this.requestExecutor = ConfigServerHttpRequestExecutor.create(configServerHosts);
-    }
-
     @Override
     public boolean suspend(final String hostName) {
-        PrefixLogger logger = PrefixLogger.getNodeAgentLogger(OrchestratorImpl.class,
-                NodeRepositoryImpl.containerNameFromHostName(hostName));
-
+        PrefixLogger logger = getLogger(hostName);
         try {
-            final UpdateHostResponse updateHostResponse = requestExecutor.put(
-                    ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName + "/suspended",
-                    WEB_SERVICE_PORT,
-                    Optional.empty(), /* body */
-                    UpdateHostResponse.class);
-            return updateHostResponse.reason() == null;
+            String path = getSuspendPath(hostName);
+            UpdateHostResponse response = requestExecutor.put(path,
+                                                              WEB_SERVICE_PORT,
+                                                              Optional.empty(), /* body */
+                                                              UpdateHostResponse.class);
+            return response.reason() == null;
         } catch (ConfigServerHttpRequestExecutor.NotFoundException n) {
             // Orchestrator doesn't care about this node, so don't let that stop us.
-            logger.info("Got not found on delete, suspending");
+            logger.info("Got not found on suspending, allowed to suspend");
             return true;
         } catch (Exception e) {
-            logger.info("Got error on suspend " + hostName, e);
+            logger.info("Got error on suspend", e);
             return false;
         }
     }
@@ -84,21 +73,27 @@ public class OrchestratorImpl implements Orchestrator {
 
     @Override
     public boolean resume(final String hostName) {
-        PrefixLogger logger = PrefixLogger.getNodeAgentLogger(OrchestratorImpl.class,
-                NodeRepositoryImpl.containerNameFromHostName(hostName));
+        PrefixLogger logger = getLogger(hostName);
         try {
-            final UpdateHostResponse batchOperationResult = requestExecutor.delete(
-                    ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName + "/suspended",
-                    WEB_SERVICE_PORT,
-                    UpdateHostResponse.class);
-            return batchOperationResult.reason() == null;
+            String path = getSuspendPath(hostName);
+            UpdateHostResponse response = requestExecutor.delete(path, WEB_SERVICE_PORT, UpdateHostResponse.class);
+            return response.reason() == null;
         } catch (ConfigServerHttpRequestExecutor.NotFoundException n) {
             // Orchestrator doesn't care about this node, so don't let that stop us.
-            logger.info("Got not found on delete, resuming");
+            logger.info("Got not found on resuming, allowed to resume");
             return true;
         } catch (Exception e) {
-            logger.info("Got error on resume " + hostName, e);
+            logger.info("Got error on resume", e);
             return false;
         }
     }
+
+    private PrefixLogger getLogger(String hostName) {
+        return PrefixLogger.getNodeAgentLogger(OrchestratorImpl.class, NodeRepositoryImpl.containerNameFromHostName(hostName));
+    }
+
+    private String getSuspendPath(String hostName) {
+        return ORCHESTRATOR_PATH_PREFIX_HOST_API + "/" + hostName + "/suspended";
+    }
+
 }
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
index c81442e5a54..021192ad533 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/provider/ComponentsProviderImpl.java
@@ -21,6 +21,7 @@ import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepository;
 import com.yahoo.vespa.hosted.node.admin.noderepository.NodeRepositoryImpl;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.Orchestrator;
 import com.yahoo.vespa.hosted.node.admin.orchestrator.OrchestratorImpl;
+import com.yahoo.vespa.hosted.node.admin.util.ConfigServerHttpRequestExecutor;
 import com.yahoo.vespa.hosted.node.admin.util.Environment;
 import com.yahoo.vespa.hosted.node.admin.util.SecretAgentScheduleMaker;
 
@@ -53,8 +54,11 @@ public class ComponentsProviderImpl implements ComponentsProvider {
                                   boolean isRunningLocally) {
         String baseHostName = HostName.getLocalhost();
         Set<String> configServerHosts = environment.getConfigServerHosts();
+        if (configServerHosts.isEmpty()) {
+            throw new IllegalStateException("Environment setting for config servers missing or empty.");
+        }
 
-        Orchestrator orchestrator = new OrchestratorImpl(configServerHosts);
+        Orchestrator orchestrator = new OrchestratorImpl(ConfigServerHttpRequestExecutor.create(configServerHosts));
         NodeRepository nodeRepository = new NodeRepositoryImpl(configServerHosts, WEB_SERVICE_PORT, baseHostName);
         DockerOperations dockerOperations = new DockerOperationsImpl(docker, environment, metricReceiver);