Fix RollingUpdateCommand to print correct rollout options

Refactor `WildcardJobCommand.runWithJobId()` to take a `Job` arg. The downside is passing more info than necessary for some subcommands. The upside is we do not make extra calls to the master to retrieve a Job we already have.
spotify · Nov 8, 2017 · 141399e · 141399e
1 parent 967b6a9
commit 141399e
Show file tree

Hide file tree

Showing 10 changed files with 89 additions and 65 deletions.
diff --git a/helios-client/src/main/java/com/spotify/helios/common/descriptors/RolloutOptions.java b/helios-client/src/main/java/com/spotify/helios/common/descriptors/RolloutOptions.java
@@ -101,10 +101,9 @@ public Builder toBuilder() {
         .setIgnoreFailures(ignoreFailures);
   }
 
-
   /**
    * Return a new RolloutOptions instance by merging this instance with another one.
-   * The other instance is assumed to have no null-valued fields.
+   * @throws NullPointerException if any attribute in both instances are null.
    */
   public RolloutOptions withFallback(final RolloutOptions that) {
     return RolloutOptions.newBuilder()

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobDeployCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobDeployCommand.java
@@ -28,6 +28,7 @@
 import com.google.common.collect.Lists;
 import com.spotify.helios.client.HeliosClient;
 import com.spotify.helios.common.descriptors.Deployment;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import com.spotify.helios.common.protocol.JobDeployResponse;
 import java.io.BufferedReader;
@@ -74,16 +75,17 @@ public JobDeployCommand(final Subparser parser) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException {
+    final JobId jobId = job.getId();
     final List<String> hosts = options.getList(hostsArg.getDest());
-    final Deployment job = Deployment.of(jobId,
+    final Deployment deployment = Deployment.of(jobId,
         options.getBoolean(noStartArg.getDest()) ? STOP : START);
 
     if (!json) {
-      out.printf("Deploying %s on %s%n", job, hosts);
+      out.printf("Deploying %s on %s%n", deployment, hosts);
     }
 
     int code = 0;
@@ -98,7 +100,7 @@ protected int runWithJobId(final Namespace options, final HeliosClient client,
         out.printf("%s: ", host);
       }
       final String token = options.getString(tokenArg.getDest());
-      final JobDeployResponse result = client.deploy(job, host, token).get();
+      final JobDeployResponse result = client.deploy(deployment, host, token).get();
       if (result.getStatus() == JobDeployResponse.Status.OK) {
         if (!json) {
           out.printf("done%n");

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobInspectCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobInspectCommand.java
@@ -138,19 +138,11 @@ public JobInspectCommand(final Subparser parser, final TimeZone timeZone) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException {
 
-    final Map<JobId, Job> jobs = client.jobs(jobId.toString()).get();
-    if (jobs.size() == 0) {
-      out.printf("Unknown job: %s%n", jobId);
-      return 1;
-    }
-
-    final Job job = Iterables.getOnlyElement(jobs.values());
-
     if (json) {
       out.println(Json.asPrettyStringUnchecked(job));
     } else {

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobRemoveCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobRemoveCommand.java
@@ -22,6 +22,7 @@
 
 import com.spotify.helios.cli.Utils;
 import com.spotify.helios.client.HeliosClient;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import com.spotify.helios.common.protocol.JobDeleteResponse;
 import java.io.BufferedReader;
@@ -54,11 +55,12 @@ public JobRemoveCommand(Subparser parser) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws IOException, ExecutionException, InterruptedException {
     final boolean yes = options.getBoolean(yesArg.getDest());
+    final JobId jobId = job.getId();
 
     if (!yes) {
       out.printf("This will remove the job %s%n", jobId);

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobStartCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobStartCommand.java
@@ -24,6 +24,7 @@
 import com.spotify.helios.client.HeliosClient;
 import com.spotify.helios.common.descriptors.Deployment;
 import com.spotify.helios.common.descriptors.Goal;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -55,11 +56,11 @@ public JobStartCommand(Subparser parser) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException, IOException {
-
+    final JobId jobId = job.getId();
     final List<String> hosts = options.getList(hostsArg.getDest());
 
     final Deployment deployment = new Deployment.Builder()

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobStopCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobStopCommand.java
@@ -24,6 +24,7 @@
 import com.spotify.helios.client.HeliosClient;
 import com.spotify.helios.common.descriptors.Deployment;
 import com.spotify.helios.common.descriptors.Goal;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import java.io.BufferedReader;
 import java.io.IOException;
@@ -55,10 +56,11 @@ public JobStopCommand(Subparser parser) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException, IOException {
+    final JobId jobId = job.getId();
     final List<String> hosts = options.getList(hostsArg.getDest());
 
     final Deployment deployment = new Deployment.Builder()

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/JobUndeployCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/JobUndeployCommand.java
@@ -25,6 +25,7 @@
 import com.google.common.collect.ImmutableList;
 import com.spotify.helios.cli.Utils;
 import com.spotify.helios.client.HeliosClient;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import com.spotify.helios.common.descriptors.JobStatus;
 import com.spotify.helios.common.protocol.JobUndeployResponse;
@@ -68,11 +69,11 @@ public JobUndeployCommand(final Subparser parser) {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException, IOException {
-
+    final JobId jobId = job.getId();
     final boolean all = options.getBoolean(allArg.getDest());
     final boolean yes = options.getBoolean(yesArg.getDest());
     final List<String> hosts;

diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/RollingUpdateCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/RollingUpdateCommand.java
@@ -21,7 +21,6 @@
 package com.spotify.helios.cli.command;
 
 import static com.google.common.base.Preconditions.checkArgument;
-import static com.spotify.helios.common.descriptors.Job.EMPTY_TOKEN;
 import static java.lang.String.format;
 import static net.sourceforge.argparse4j.impl.Arguments.storeTrue;
 
@@ -31,6 +30,7 @@
 import com.google.common.collect.Sets;
 import com.spotify.helios.client.HeliosClient;
 import com.spotify.helios.common.Json;
+import com.spotify.helios.common.descriptors.Job;
 import com.spotify.helios.common.descriptors.JobId;
 import com.spotify.helios.common.descriptors.RolloutOptions;
 import com.spotify.helios.common.descriptors.TaskStatus;
@@ -144,10 +144,12 @@ public Long get() {
   }
 
   @Override
-  protected int runWithJobId(final Namespace options, final HeliosClient client,
-                             final PrintStream out, final boolean json, final JobId jobId,
-                             final BufferedReader stdin)
+  protected int runWithJob(final Namespace options, final HeliosClient client,
+                           final PrintStream out, final boolean json, final Job job,
+                           final BufferedReader stdin)
       throws ExecutionException, InterruptedException, IOException {
+    final JobId jobId = job.getId();
+
     final String name = options.getString(nameArg.getDest());
     final Long timeout = options.getLong(timeoutArg.getDest());
     final Integer parallelism = options.getInt(parallelismArg.getDest());
@@ -184,27 +186,36 @@ protected int runWithJobId(final Namespace options, final HeliosClient client,
       return 1;
     }
 
+    final RolloutOptions optionsFromJob = job.getRolloutOptions();
+    final Integer actualParallelism =
+        nullableWithFallback(parallelism, optionsFromJob.getParallelism());
+    final Long actualTimeout = nullableWithFallback(timeout, optionsFromJob.getTimeout());
+    final Boolean actualOverlap = nullableWithFallback(overlap, optionsFromJob.getOverlap());
+    final String actualToken = nullableWithFallback(token, optionsFromJob.getToken());
+    final Boolean actualIgnoreFailures =
+        nullableWithFallback(ignoreFailures, optionsFromJob.getIgnoreFailures());
+
     if (!json) {
       out.println(format("Rolling update%s started: %s -> %s "
                          + "(parallelism=%d, timeout=%d, overlap=%b, token=%s, "
                          + "ignoreFailures=%b)%s",
           async ? " (async)" : "",
           name,
           jobId.toShortString(),
-          parallelism,
-          timeout,
-          overlap,
-          token,
-          ignoreFailures,
+          actualParallelism,
+          actualTimeout,
+          actualOverlap,
+          actualToken,
+          actualIgnoreFailures,
           async ? "" : "\n"));
     }
 
     final Map<String, Object> jsonOutput = Maps.newHashMap();
-    jsonOutput.put("parallelism", parallelism);
-    jsonOutput.put("timeout", timeout);
-    jsonOutput.put("overlap", overlap);
-    jsonOutput.put("token", token);
-    jsonOutput.put("ignoreFailures", ignoreFailures);
+    jsonOutput.put("parallelism", actualParallelism);
+    jsonOutput.put("timeout", actualTimeout);
+    jsonOutput.put("overlap", actualOverlap);
+    jsonOutput.put("token", actualToken);
+    jsonOutput.put("ignoreFailures", actualIgnoreFailures);
 
     if (async) {
       if (json) {
@@ -298,5 +309,12 @@ protected int runWithJobId(final Namespace options, final HeliosClient client,
   interface SleepFunction {
     void sleep(long millis) throws InterruptedException;
   }
-}
 
+  /**
+   * Return first argument if not null. Otherwise return second argument.
+   */
+  private <T> T nullableWithFallback(final T first, final T second) {
+    return first != null ? first : second;
+  }
+
+}
diff --git a/helios-tools/src/main/java/com/spotify/helios/cli/command/WildcardJobCommand.java b/helios-tools/src/main/java/com/spotify/helios/cli/command/WildcardJobCommand.java
@@ -81,13 +81,13 @@ int run(final Namespace options, final HeliosClient client, final PrintStream ou
       return 1;
     }
 
-    final JobId jobId = Iterables.getOnlyElement(jobs.keySet());
+    final Job job = Iterables.getOnlyElement(jobs.values());
 
-    return runWithJobId(options, client, out, json, jobId, stdin);
+    return runWithJob(options, client, out, json, job, stdin);
   }
 
-  protected abstract int runWithJobId(final Namespace options, final HeliosClient client,
-                                      final PrintStream out, final boolean json, final JobId jobId,
-                                      final BufferedReader stdin)
+  protected abstract int runWithJob(final Namespace options, final HeliosClient client,
+                                    final PrintStream out, final boolean json, final Job job,
+                                    final BufferedReader stdin)
       throws ExecutionException, InterruptedException, IOException;
 }