Make ApplicationChange mandatory on JobStatus

10 files changed, 32 insertions, 45 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
index 89b134cc228..603f6c3ad12 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/ApplicationController.java
@@ -336,7 +336,7 @@ public class ApplicationController {
                                                                 application.deploying(),
                                                                 triggering.at(),
                                                                 version,
-                                                                Optional.of(applicationVersion),
+                                                                applicationVersion,
                                                                 triggering.reason());
                 }
             }
@@ -474,7 +474,7 @@ public class ApplicationController {
     }
 
     private JobStatus.JobRun incompleteTriggeringEvent(Version version) {
-        return new JobStatus.JobRun(-1, version, Optional.empty(), false, "", clock.instant());
+        return new JobStatus.JobRun(-1, version, ApplicationVersion.unknown, false, "", clock.instant());
     }
 
     private DeployOptions withVersion(Version version, DeployOptions options) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
index b50ecb82c50..e84409296dd 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/LockedApplication.java
@@ -65,7 +65,7 @@ public class LockedApplication extends Application {
     }
 
     public LockedApplication withJobTriggering(JobType type, Optional<Change> change, Instant triggerTime,
-                                               Version version, Optional<ApplicationVersion> applicationVersion,
+                                               Version version, ApplicationVersion applicationVersion,
                                                String reason) {
         return new LockedApplication(new Builder(this).with(deploymentJobs().withTriggering(type, change, version, applicationVersion, reason, triggerTime)));
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
index c432503a790..677f7809c33 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/DeploymentJobs.java
@@ -67,7 +67,7 @@ public class DeploymentJobs {
     public DeploymentJobs withTriggering(JobType jobType,
                                          Optional<Change> change,
                                          Version version,
-                                         Optional<ApplicationVersion> applicationVersion,
+                                         ApplicationVersion applicationVersion,
                                          String reason,
                                          Instant triggerTime) {
         Map<JobType, JobStatus> status = new LinkedHashMap<>(this.status);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
index 932229b6bb6..41060a7af4c 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobList.java
@@ -172,7 +172,7 @@ public class JobList {
     private static boolean failingApplicationChange(JobStatus job) {
         if (   job.isSuccess()) return false;
         if ( ! job.lastSuccess().isPresent()) return true; // An application which never succeeded is surely bad.
-        if ( ! job.lastSuccess().get().applicationVersion().isPresent()) return true; // Indicates the component job, which is always an application change.
+        if ( job.lastSuccess().get().applicationVersion() == ApplicationVersion.unknown) return true; // Indicates the component job, which is always an application change.
         if ( ! job.firstFailing().get().version().equals(job.lastSuccess().get().version())) return false; // Version change may be to blame.
         return ! job.firstFailing().get().applicationVersion().equals(job.lastSuccess().get().applicationVersion()); // Return whether there is an application change.
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
index 6a29d4990e7..10463e6d296 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/application/JobStatus.java
@@ -57,25 +57,18 @@ public class JobStatus {
 
     public JobStatus withTriggering(Version version, ApplicationVersion applicationVersion,
                                     boolean upgrade, String reason, Instant triggerTime) {
-        return withTriggering(version,
-                              applicationVersion == ApplicationVersion.unknown ? Optional.empty() : Optional.of(applicationVersion),
-                              upgrade, reason, triggerTime);
-    }
-
-    public JobStatus withTriggering(Version version, Optional<ApplicationVersion> applicationVersion,
-                                    boolean upgrade, String reason, Instant triggerTime) {
         return new JobStatus(type, jobError, Optional.of(new JobRun(-1, version, applicationVersion, upgrade, reason, triggerTime)),
                              lastCompleted, firstFailing, lastSuccess);
     }
 
     public JobStatus withCompletion(long runId, Optional<DeploymentJobs.JobError> jobError, Instant completionTime, Controller controller) {
         Version version;
-        Optional<ApplicationVersion> applicationVersion;
+        ApplicationVersion applicationVersion;
         boolean upgrade;
         String reason;
         if (type == DeploymentJobs.JobType.component) { // not triggered by us
             version = controller.systemVersion();
-            applicationVersion = Optional.empty();
+            applicationVersion = ApplicationVersion.unknown;
             upgrade = false;
             reason = "Application commit";
         }
@@ -174,13 +167,12 @@ public class JobStatus {
 
         private final long id;
         private final Version version;
-        // TODO: Make non-optional after introducing new application version number
-        private final Optional<ApplicationVersion> applicationVersion;
+        private final ApplicationVersion applicationVersion;
         private final boolean upgrade;
         private final String reason;
         private final Instant at;
 
-        public JobRun(long id, Version version, Optional<ApplicationVersion> applicationVersion,
+        public JobRun(long id, Version version, ApplicationVersion applicationVersion,
                       boolean upgrade, String reason, Instant at) {
             Objects.requireNonNull(version, "version cannot be null");
             Objects.requireNonNull(applicationVersion, "applicationVersion cannot be null");
@@ -206,7 +198,7 @@ public class JobStatus {
         public Version version() { return version; }
 
         /** Returns the application version used for this run, or empty when not known */
-        public Optional<ApplicationVersion> applicationVersion() { return applicationVersion; }
+        public ApplicationVersion applicationVersion() { return applicationVersion; }
 
         /** Returns a human-readable reason for this particular job run */
         public String reason() { return reason; }
@@ -218,14 +210,9 @@ public class JobStatus {
         /** Returns whether the job last completed for the given change */
         public boolean lastCompletedWas(Change change) {
             if (change instanceof Change.ApplicationChange) {
-                Change.ApplicationChange applicationChange = (Change.ApplicationChange) change;
-                if ( ! applicationVersion().isPresent())
-                    return  applicationChange.version() == ApplicationVersion.unknown;
-                else
-                    return applicationVersion().get().equals(applicationChange.version());
+                return ((Change.ApplicationChange) change).version().equals(applicationVersion);
             } else if (change instanceof Change.VersionChange) {
-                Change.VersionChange versionChange = (Change.VersionChange) change;
-                return version().equals(versionChange.version());
+                return version().equals(((Change.VersionChange) change).version());
             }
             throw new IllegalArgumentException("Unexpected change: " + change.getClass());
         }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
index 6fd25f1a8c6..f02b46fdbb1 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTrigger.java
@@ -255,7 +255,7 @@ public class DeploymentTrigger {
         else { // Application version changes do not need to handle downgrading
             if ( ! previous.lastSuccess().isPresent()) return false;
             if ( ! next.lastSuccess().isPresent()) return true;
-            return previous.lastSuccess().get().applicationVersion().isPresent() &&
+            return previous.lastSuccess().get().applicationVersion() != ApplicationVersion.unknown &&
                    ! previous.lastSuccess().get().applicationVersion().equals(next.lastSuccess().get().applicationVersion());
         }
     }
@@ -367,7 +367,7 @@ public class DeploymentTrigger {
                                              application.deploying(),
                                              clock.instant(),
                                              application.deployVersionFor(jobType, controller),
-                                             application.deployApplicationVersion(jobType, controller),
+                                             application.deployApplicationVersion(jobType, controller).orElse(ApplicationVersion.unknown),
                                              reason);
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
index 1664e984a27..0747d24a80a 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializer.java
@@ -236,8 +236,8 @@ public class ApplicationSerializer {
         Cursor object = parent.setObject(jobRunObjectName);
         object.setLong(jobRunIdField, jobRun.get().id());
         object.setString(versionField, jobRun.get().version().toString());
-        if ( jobRun.get().applicationVersion().isPresent())
-            toSlime(jobRun.get().applicationVersion().get(), object.setObject(revisionField));
+        if ( jobRun.get().applicationVersion() != ApplicationVersion.unknown)
+            toSlime(jobRun.get().applicationVersion(), object.setObject(revisionField));
         object.setBool(upgradeField, jobRun.get().upgrade());
         object.setString(reasonField, jobRun.get().reason());
         object.setLong(atField, jobRun.get().at().toEpochMilli());
@@ -282,7 +282,7 @@ public class ApplicationSerializer {
 
     private Deployment deploymentFromSlime(Inspector deploymentObject) {
         return new Deployment(zoneIdFromSlime(deploymentObject.field(zoneField)),
-                              applicationVersionFromSlime(deploymentObject.field(applicationPackageRevisionField)).get(),
+                              applicationVersionFromSlime(deploymentObject.field(applicationPackageRevisionField)),
                               Version.fromString(deploymentObject.field(versionField).asString()),
                               Instant.ofEpochMilli(deploymentObject.field(deployTimeField).asLong()),
                               clusterUtilsMapFromSlime(deploymentObject.field(clusterUtilsField)),
@@ -340,12 +340,12 @@ public class ApplicationSerializer {
         return ZoneId.from(object.field(environmentField).asString(), object.field(regionField).asString());
     }
 
-    private Optional<ApplicationVersion> applicationVersionFromSlime(Inspector object) {
-        if ( ! object.valid()) return Optional.empty();
+    private ApplicationVersion applicationVersionFromSlime(Inspector object) {
+        if ( ! object.valid()) return ApplicationVersion.unknown;
         String applicationPackageHash = object.field(applicationPackageHashField).asString();
         Optional<SourceRevision> sourceRevision = sourceRevisionFromSlime(object.field(sourceRevisionField));
-        return sourceRevision.isPresent() ? Optional.of(ApplicationVersion.from(applicationPackageHash, sourceRevision.get()))
-                                          : Optional.of(ApplicationVersion.from(applicationPackageHash));
+        return sourceRevision.isPresent() ? ApplicationVersion.from(applicationPackageHash, sourceRevision.get())
+                                          : ApplicationVersion.from(applicationPackageHash);
     }
 
     private Optional<SourceRevision> sourceRevisionFromSlime(Inspector object) {
@@ -369,7 +369,7 @@ public class ApplicationSerializer {
         if (versionFieldValue.valid())
             return Optional.of(new Change.VersionChange(Version.fromString(versionFieldValue.asString())));
         else if (object.field(applicationPackageHashField).valid())
-            return Optional.of(Change.ApplicationChange.of(applicationVersionFromSlime(object).get()));
+            return Optional.of(Change.ApplicationChange.of(applicationVersionFromSlime(object)));
         else
             return Optional.of(Change.ApplicationChange.unknown());
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
index 3cca0fb6318..5afd89846b6 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiHandler.java
@@ -970,8 +970,8 @@ public class ApplicationApiHandler extends LoggingRequestHandler {
     private void toSlime(JobStatus.JobRun jobRun, Cursor object) {
         object.setLong("id", jobRun.id());
         object.setString("version", jobRun.version().toFullString());
-        jobRun.applicationVersion().ifPresent(applicationVersion -> toSlime(applicationVersion,
-                                                                            object.setObject("revision")));
+        if (jobRun.applicationVersion() != ApplicationVersion.unknown)
+            toSlime(jobRun.applicationVersion(), object.setObject("revision"));
         object.setString("reason", jobRun.reason());
         object.setLong("at", jobRun.at().toEpochMilli());
     }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
index 8c242ef1150..063ccc79fe5 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTest.java
@@ -231,12 +231,12 @@ public class ControllerTest {
         tester.deployAndNotify(app1, true, stagingTest);
         assertEquals(4, applications.require(app1.id()).deploymentJobs().jobStatus().size());
         assertStatus(JobStatus.initial(stagingTest)
-                              .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
+                              .withTriggering(version1, expectedVersion, false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
                               .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
 
         // Causes first deployment job to be triggered
         assertStatus(JobStatus.initial(productionCorpUsEast1)
-                              .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant()), app1.id(), tester.controller());
+                              .withTriggering(version1, expectedVersion, false, "", tester.clock().instant()), app1.id(), tester.controller());
         tester.clock().advance(Duration.ofSeconds(1));
 
         // production job (failing)
@@ -244,7 +244,7 @@ public class ControllerTest {
         assertEquals(4, applications.require(app1.id()).deploymentJobs().jobStatus().size());
 
         JobStatus expectedJobStatus = JobStatus.initial(productionCorpUsEast1)
-                                               .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant())
+                                               .withTriggering(version1, expectedVersion, false, "", tester.clock().instant())
                                                .withCompletion(42, Optional.of(JobError.unknown), tester.clock().instant(), tester.controller());
 
         assertStatus(expectedJobStatus, app1.id(), tester.controller());
@@ -270,20 +270,20 @@ public class ControllerTest {
         tester.deployAndNotify(app1, Optional.empty(), true, false, systemTest);
         expectedVersion = ApplicationVersion.from(source, 38);
         assertStatus(JobStatus.initial(systemTest)
-                              .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
+                              .withTriggering(version1, expectedVersion, false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
                               .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
         tester.deployAndNotify(app1, Optional.empty(), true, true, stagingTest);
 
         // production job succeeding now
         tester.deployAndNotify(app1, Optional.empty(), true, true, productionCorpUsEast1);
         expectedJobStatus = expectedJobStatus
-                .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
+                .withTriggering(version1, expectedVersion, false, "", tester.clock().instant().minus(Duration.ofMillis(1)))
                 .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller());
         assertStatus(expectedJobStatus, app1.id(), tester.controller());
 
         // causes triggering of next production job
         assertStatus(JobStatus.initial(productionUsEast3)
-                              .withTriggering(version1, Optional.of(expectedVersion), false, "", tester.clock().instant()),
+                              .withTriggering(version1, expectedVersion, false, "", tester.clock().instant()),
                      app1.id(), tester.controller());
         tester.deployAndNotify(app1, Optional.empty(), true, true, productionUsEast3);
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
index 3b099d7e7ed..1e00ecc2107 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/persistence/ApplicationSerializerTest.java
@@ -70,10 +70,10 @@ public class ApplicationSerializerTest {
         List<JobStatus> statusList = new ArrayList<>();
 
         statusList.add(JobStatus.initial(DeploymentJobs.JobType.systemTest)
-                                .withTriggering(Version.fromString("5.6.7"), Optional.empty(), true, "Test", Instant.ofEpochMilli(7))
+                                .withTriggering(Version.fromString("5.6.7"), ApplicationVersion.unknown, true, "Test", Instant.ofEpochMilli(7))
                                 .withCompletion(30, Optional.empty(), Instant.ofEpochMilli(8), tester.controller()));
         statusList.add(JobStatus.initial(DeploymentJobs.JobType.stagingTest)
-                                .withTriggering(Version.fromString("5.6.6"), Optional.empty(), true, "Test 2", Instant.ofEpochMilli(5))
+                                .withTriggering(Version.fromString("5.6.6"), ApplicationVersion.unknown, true, "Test 2", Instant.ofEpochMilli(5))
                                 .withCompletion(11, Optional.of(JobError.unknown), Instant.ofEpochMilli(6), tester.controller()));
 
         DeploymentJobs deploymentJobs = new DeploymentJobs(projectId, statusList, Optional.empty());