Merge pull request #4046 from vespa-engine/mpolden/fix-jobrun-equals

Fix JobRun equals

11 files changed, 18 insertions, 46 deletions

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 929bf186eea..47fe2231db0 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
@@ -4,7 +4,6 @@ package com.yahoo.vespa.hosted.controller.application;
 import com.yahoo.component.Version;
 import com.yahoo.vespa.hosted.controller.Controller;
 
-import java.time.Duration;
 import java.time.Instant;
 import java.util.Objects;
 import java.util.Optional;
@@ -220,7 +219,7 @@ public class JobStatus {
             if (this == o) return true;
             if ( ! (o instanceof JobRun)) return false;
             JobRun jobRun = (JobRun) o;
-            return id == id &&
+            return id == jobRun.id &&
                    Objects.equals(version, jobRun.version) &&
                    Objects.equals(revision, jobRun.revision) &&
                    upgrade == jobRun.upgrade &&
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
index e4f4bd2ee96..5b87f9eaa86 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/Upgrader.java
@@ -26,8 +26,6 @@ import java.util.logging.Logger;
  */
 public class Upgrader extends Maintainer {
 
-    private static final Duration upgradeTimeout = Duration.ofHours(12);
-
     private static final Logger log = Logger.getLogger(Upgrader.class.getName());
 
     private final CuratorDb curator;
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 2745862f68a..04394f08e64 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
@@ -202,9 +202,7 @@ public class ApplicationSerializer {
     }
     
     private void toSlime(DeploymentJobs deploymentJobs, Cursor cursor) {
-        deploymentJobs.projectId()
-                .filter(id -> id > 0) // TODO: Discards invalid data. Remove filter after October 2017
-                .ifPresent(projectId -> cursor.setLong(projectIdField, projectId));
+        deploymentJobs.projectId().ifPresent(projectId -> cursor.setLong(projectIdField, projectId));
         jobStatusToSlime(deploymentJobs.jobStatus().values(), cursor.setArray(jobStatusField));
         deploymentJobs.issueId().ifPresent(jiraIssueId -> cursor.setString(issueIdField, jiraIssueId.value()));
     }
@@ -347,8 +345,7 @@ public class ApplicationSerializer {
     }
 
     private DeploymentJobs deploymentJobsFromSlime(Inspector object) {
-        Optional<Long> projectId = optionalLong(object.field(projectIdField))
-                .filter(id -> id > 0); // TODO: Discards invalid data. Remove filter after October 2017
+        Optional<Long> projectId = optionalLong(object.field(projectIdField));
         List<JobStatus> jobStatusList = jobStatusListFromSlime(object.field(jobStatusField));
         Optional<IssueId> issueId = optionalString(object.field(issueIdField)).map(IssueId::from);
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
index 554c496bc71..e5616f025ce 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/CuratorDb.java
@@ -176,10 +176,6 @@ public class CuratorDb {
         VersionStatusSerializer serializer = new VersionStatusSerializer();
         NestedTransaction transaction = new NestedTransaction();
         try {
-            // TODO: Removes unused data. Remove after October 2017
-            if (curator.getData(systemVersionPath()).isPresent()) {
-                curator.delete(systemVersionPath());
-            }
             curator.set(versionStatusPath(), SlimeUtils.toJsonBytes(serializer.toSlime(status)));
         } catch (IOException e) {
             throw new UncheckedIOException("Failed to serialize version status", e);
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java
index 37677a5e393..ab240b9dea9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/persistence/MemoryControllerDb.java
@@ -24,9 +24,9 @@ import java.util.stream.Collectors;
  */
 public class MemoryControllerDb extends ControllerDb {
 
-    private Map<TenantId, Tenant> tenants = new HashMap<>();
-    private Map<String, Application> applications = new HashMap<>();
-    private Map<RotationId, ApplicationId> rotationAssignments = new HashMap<>();
+    private final Map<TenantId, Tenant> tenants = new HashMap<>();
+    private final Map<String, Application> applications = new HashMap<>();
+    private final Map<RotationId, ApplicationId> rotationAssignments = new HashMap<>();
 
     @Override
     public void createTenant(Tenant tenant) {
@@ -46,23 +46,14 @@ public class MemoryControllerDb extends ControllerDb {
 
     @Override
     public void deleteTenant(TenantId tenantId) {
-        Object removed = tenants.remove(tenantId);
-        if (removed == null)
+        if (tenants.remove(tenantId) == null) {
             throw new NotExistsException(tenantId);
+        }
     }
 
     @Override
     public Optional<Tenant> getTenant(TenantId tenantId) throws PersistenceException {
-        Optional<Tenant> tenant = Optional.ofNullable(tenants.get(tenantId));
-        if(tenant.isPresent()) {
-            Tenant t_noquota = tenant.get();
-            Tenant t_withquota = new Tenant(
-                    t_noquota.getId(), t_noquota.getUserGroup(), t_noquota.getProperty(),
-                    t_noquota.getAthensDomain(), t_noquota.getPropertyId());
-            return Optional.of(t_withquota);
-        } else {
-            return tenant;
-        }
+        return Optional.ofNullable(tenants.get(tenantId));
     }
 
     @Override
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
index 529ac292d8b..27b219cd892 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/deployment/DeploymentApiHandler.java
@@ -35,6 +35,7 @@ import static java.util.Comparator.comparing;
  *
  * @author bratseth
  */
+@SuppressWarnings("unused") // Injected
 public class DeploymentApiHandler extends LoggingRequestHandler {
 
     private final Controller controller;
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlHeaders.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlHeaders.java
index aea59c16cd5..8a539720a21 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlHeaders.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/AccessControlHeaders.java
@@ -3,16 +3,15 @@ package com.yahoo.vespa.hosted.controller.restapi.filter;
 
 import com.google.common.collect.ImmutableMap;
 
+import java.time.Duration;
 import java.util.Map;
 
-import static java.util.concurrent.TimeUnit.DAYS;
-
 /**
  * @author gv
  */
 public interface AccessControlHeaders {
 
-    String CORS_PREFLIGHT_REQUEST_CACHE_TTL =  Long.toString(DAYS.toSeconds(7));
+    String CORS_PREFLIGHT_REQUEST_CACHE_TTL =  Long.toString(Duration.ofDays(7).getSeconds());
 
     String ALLOW_ORIGIN_HEADER = "Access-Control-Allow-Origin";
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/securitycontext/CreateSecurityContextFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/securitycontext/CreateSecurityContextFilter.java
index 850130ca970..6073307bafa 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/securitycontext/CreateSecurityContextFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/securitycontext/CreateSecurityContextFilter.java
@@ -19,6 +19,7 @@ import java.security.Principal;
  */
 @After("BouncerFilter")
 @Provides("SecurityContext")
+@SuppressWarnings("unused") // Injected
 public class CreateSecurityContextFilter implements SecurityRequestFilter {
 
     @Override
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 7875c1f4964..a0ada7adab6 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
@@ -30,7 +30,6 @@ import com.yahoo.vespa.hosted.controller.application.ApplicationRevision;
 import com.yahoo.vespa.hosted.controller.application.Change;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobError;
-import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobReport;
 import com.yahoo.vespa.hosted.controller.application.DeploymentJobs.JobType;
 import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.athenz.NToken;
@@ -105,7 +104,7 @@ public class ControllerTest {
         assertTrue("Revision has been set during deployment", revision.isPresent());
         assertStatus(JobStatus.initial(stagingTest)
                               .withTriggering(-1, version1, revision, false, "", tester.clock().instant())
-                              .withCompletion(-1, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
+                              .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
 
         // Causes first deployment job to be triggered
         assertStatus(JobStatus.initial(productionCorpUsEast1)
@@ -118,7 +117,7 @@ public class ControllerTest {
 
         JobStatus expectedJobStatus = JobStatus.initial(productionCorpUsEast1)
                                                .withTriggering(-1, version1, revision, false, "", tester.clock().instant()) // Triggered first without revision info
-                                               .withCompletion(-1, Optional.of(JobError.unknown), tester.clock().instant(), tester.controller())
+                                               .withCompletion(42, Optional.of(JobError.unknown), tester.clock().instant(), tester.controller())
                                                .withTriggering(-1, version1, revision, false, "", tester.clock().instant()); // Re-triggering (due to failure) has revision info
                 
         assertStatus(expectedJobStatus, app1.id(), tester.controller());
@@ -140,14 +139,14 @@ public class ControllerTest {
         tester.deployAndNotify(app1, applicationPackage, true, false, systemTest);
         assertStatus(JobStatus.initial(systemTest)
                               .withTriggering(-1, version1, revision, false, "", tester.clock().instant())
-                              .withCompletion(-1, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
+                              .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller()), app1.id(), tester.controller());
         tester.deployAndNotify(app1, applicationPackage, true, stagingTest);
 
         // production job succeeding now
         tester.deployAndNotify(app1, applicationPackage, true, productionCorpUsEast1);
         expectedJobStatus = expectedJobStatus
                 .withTriggering(-1, version1, revision, false, "", tester.clock().instant())
-                .withCompletion(-1, Optional.empty(), tester.clock().instant(), tester.controller());
+                .withCompletion(42, Optional.empty(), tester.clock().instant(), tester.controller());
         assertStatus(expectedJobStatus, app1.id(), tester.controller());
 
         // causes triggering of next production job
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/pr-instance-with-dead-locked-job.json b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/pr-instance-with-dead-locked-job.json
index 32d34edd576..425b9d4512d 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/pr-instance-with-dead-locked-job.json
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/pr-instance-with-dead-locked-job.json
@@ -4,7 +4,7 @@
   "validationOverrides": "<deployment version='1.0'/>",
   "deployments": [],
   "deploymentJobs": {
-    "projectId": 0,
+    "projectId": 42,
     "jobStatus": [
       {
         "jobType": "system-test",
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 b38a38c3120..c408f2ee214 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
@@ -200,15 +200,6 @@ public class ApplicationSerializerTest {
         assertFalse(application.deploymentJobs().jobStatus().get(DeploymentJobs.JobType.systemTest).lastCompleted().get().upgrade());
     }
 
-    // TODO: Remove after October 2017
-    @Test
-    public void testLegacySerializationWithZeroProjectId() {
-        Application original = applicationSerializer.fromSlime(applicationSlime(0, false));
-        assertFalse(original.deploymentJobs().projectId().isPresent());
-        Application serialized = applicationSerializer.fromSlime(applicationSerializer.toSlime(original));
-        assertFalse(serialized.deploymentJobs().projectId().isPresent());
-    }
-
     private Slime applicationSlime(boolean error) {
         return applicationSlime(123, error);
     }