Made accessors for Lock and LockedApplication more private

10 files changed, 52 insertions, 98 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 12c0c5fd36b..565c92f3a28 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
@@ -64,7 +64,6 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Consumer;
-import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -123,8 +122,8 @@ public class ApplicationController {
 
 
     /** Returns an locked application with the given id that be updated and stored */
-    public Optional<LockedApplication> get(ApplicationId id, Lock lock) {
-        return db.getApplication(id).map(application -> new LockedApplication(application, lock));
+    private Optional<LockedApplication> get(ApplicationId id, Lock lock) {
+        return get(id).map(application -> new LockedApplication(application, lock));
     }
 
     /**
@@ -136,16 +135,6 @@ public class ApplicationController {
         return get(id).orElseThrow(() -> new IllegalArgumentException(id + " not found"));
     }
 
-    /**
-     * Returns a locked application that be updated and stored
-     *
-     * @throws IllegalArgumentException if it does not exist
-     *
-     */
-    public LockedApplication require(ApplicationId id, Lock lock) {
-        return get(id, lock).orElseThrow(() -> new IllegalArgumentException(id + " not found"));
-    }
-
     /** Returns a snapshot of all applications */
     public List<Application> asList() { 
         return db.listApplications();
@@ -156,16 +145,6 @@ public class ApplicationController {
         return db.listApplications(new TenantId(tenant.value()));
     }
 
-    /** Returns an immutable list with the ids of all known applications. */
-    public List<ApplicationId> idList() {
-        return ImmutableList.copyOf(asList().stream().map(Application::id)::iterator);
-    }
-
-    /** Returns an immutable list with the ids of all applications of the tenant. */
-    public List<ApplicationId> idList(TenantName tenant) {
-        return ImmutableList.copyOf(asList(tenant).stream().map(Application::id)::iterator);
-    }
-
     /**
      * Set the rotations marked as 'global' either 'in' or 'out of' service.
      *
@@ -565,7 +544,7 @@ public class ApplicationController {
      */
     public void lockedOrThrow(ApplicationId applicationId, Consumer<LockedApplication> actions) {
         try (Lock lock = lock(applicationId)) {
-            actions.accept(require(applicationId, lock));
+            actions.accept(get(applicationId, lock).orElseThrow(() -> new IllegalArgumentException(applicationId + " not found")));
         }
     }
 
@@ -608,20 +587,17 @@ public class ApplicationController {
 
     private void deactivate(Application application, Zone zone, Optional<Deployment> deployment,
                             boolean requireThatDeploymentHasExpired) {
-        try (Lock lock = lock(application.id())) {
-            LockedApplication lockedApplication = controller.applications().require(application.id(), lock);
-            if (deployment.isPresent() && requireThatDeploymentHasExpired && 
-                ! DeploymentExpirer.hasExpired(controller.zoneRegistry(), deployment.get(), clock.instant())) {
-                return;
-            }
-            lockedApplication = deactivate(lockedApplication, zone);
-            store(lockedApplication);
-        }
+        if (requireThatDeploymentHasExpired && deployment.isPresent()
+            && ! DeploymentExpirer.hasExpired(controller.zoneRegistry(), deployment.get(), clock.instant()))
+            return;
+
+        lockedOrThrow(application.id(), lockedApplication ->
+                store(deactivate(lockedApplication, zone)));
     }
 
-    /** 
+    /**
      * Deactivates a locked application without storing it
-     * 
+     *
      * @return the application with the deployment in the given zone removed
      */
     private LockedApplication deactivate(LockedApplication application, Zone zone) {
@@ -637,19 +613,19 @@ public class ApplicationController {
     public DeploymentTrigger deploymentTrigger() { return deploymentTrigger; }
 
     private ApplicationId dashToUnderscore(ApplicationId id) {
-        return ApplicationId.from(id.tenant().value(), 
+        return ApplicationId.from(id.tenant().value(),
                                   id.application().value().replaceAll("-", "_"),
                                   id.instance().value());
     }
-    
+
     public ConfigServerClient configserverClient() { return configserverClient; }
-    
-    /** 
+
+    /**
      * Returns a lock which provides exclusive rights to changing this application.
      * Any operation which stores an application need to first acquire this lock, then read, modify
      * and store the application, and finally release (close) the lock.
      */
-    public Lock lock(ApplicationId application) {
+    Lock lock(ApplicationId application) {
         return curator.lock(application, Duration.ofMinutes(10));
     }
 
@@ -661,18 +637,18 @@ public class ApplicationController {
     }
 
     private static final class ApplicationRotation {
-        
+
         private final ImmutableSet<String> cnames;
         private final ImmutableSet<Rotation> rotations;
-        
+
         public ApplicationRotation(Set<String> cnames, Set<Rotation> rotations) {
             this.cnames = ImmutableSet.copyOf(cnames);
             this.rotations = ImmutableSet.copyOf(rotations);
         }
-        
+
         public Set<String> cnames() { return cnames; }
         public Set<Rotation> rotations() { return rotations; }
-        
+
     }
-    
+
 }
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 5899c95015f..a39a549fc28 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
@@ -53,7 +53,7 @@ public class LockedApplication extends Application {
         return new LockedApplication(new Builder(this).with(deploymentJobs().withProjectId(projectId)));
     }
 
-    public LockedApplication with(IssueId issueId) {
+    public LockedApplication withDeploymentIssueId(IssueId issueId) {
         return new LockedApplication(new Builder(this).with(deploymentJobs().with(issueId)));
     }
 
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 6246459441b..1eee727214b 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
@@ -238,12 +238,10 @@ public class DeploymentTrigger {
      * @param applicationId the application to trigger
      */
     public void cancelChange(ApplicationId applicationId) {
-        try (Lock lock = applications().lock(applicationId)) {
-            LockedApplication application = applications().require(applicationId, lock);
+        applications().lockedOrThrow(applicationId, application -> {
             buildSystem.removeJobs(application.id());
-            application = application.withDeploying(Optional.empty());
-            applications().store(application);
-        }
+            applications().store(application.withDeploying(Optional.empty()));
+        });
     }
 
     //--- End of methods which triggers deployment jobs ----------------------------
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
index 91376bd9e80..ae6ba364d25 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentIssueReporter.java
@@ -131,11 +131,8 @@ public class DeploymentIssueReporter extends Maintainer {
     }
 
     private void store(ApplicationId id, IssueId issueId) {
-        try (Lock lock = controller().applications().lock(id)) {
-            controller().applications().get(id, lock).ifPresent(
-                    application -> controller().applications().store(application.withDeploymentIssueId(issueId))
-            );
-        }
+        controller().applications().lockedIfPresent(id, application ->
+                controller().applications().store(application.withDeploymentIssueId(issueId)));
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
index 0141179a942..13eb5075f34 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/DeploymentMetricsMaintainer.java
@@ -34,11 +34,8 @@ public class DeploymentMetricsMaintainer extends Maintainer {
         boolean hasWarned = false;
         for (Application application : ApplicationList.from(controller().applications().asList()).notPullRequest().asList()) {
             try {
-                try (Lock lock = controller().applications().lock(application.id())) {
-                    controller().applications().get(application.id(), lock)
-                            .ifPresent(lockedApplication -> controller().applications().store(
-                                    lockedApplication.with(controller().metricsService().getApplicationMetrics(application.id()))));
-                }
+                controller().applications().lockedIfPresent(application.id(), lockedApplication ->
+                        controller().applications().store(lockedApplication.with(controller().metricsService().getApplicationMetrics(application.id()))));
 
                 for (Deployment deployment : application.deployments().values()) {
                     MetricsService.DeploymentMetrics deploymentMetrics = controller().metricsService()
@@ -49,11 +46,8 @@ public class DeploymentMetricsMaintainer extends Maintainer {
                                                                          deploymentMetrics.queryLatencyMillis(),
                                                                          deploymentMetrics.writeLatencyMillis());
 
-                    try (Lock lock = controller().applications().lock(application.id())) {
-                        controller().applications().get(application.id(), lock)
-                                .ifPresent(lockedApplication -> controller().applications().store(
-                                        lockedApplication.with(deployment.zone(), appMetrics)));
-                    }
+                    controller().applications().lockedIfPresent(application.id(), lockedApplication ->
+                            controller().applications().store(lockedApplication.with(deployment.zone(), appMetrics)));
                 }
             }
             catch (UncheckedIOException e) {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
index a92e2533092..e350b98adb9 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiHandler.java
@@ -103,13 +103,13 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
     }
 
     private HttpResponse trigger(HttpRequest request, String tenantName, String applicationName) {
+        JobType jobType = Optional.of(asString(request.getData()))
+                .filter(s -> !s.isEmpty())
+                .map(JobType::fromJobName)
+                .orElse(JobType.component);
+
         ApplicationId applicationId = ApplicationId.from(tenantName, applicationName, "default");
-        try (Lock lock = controller.applications().lock(applicationId)) {
-            LockedApplication application = controller.applications().require(applicationId, lock);
-            JobType jobType = Optional.of(asString(request.getData()))
-                    .filter(s -> !s.isEmpty())
-                    .map(JobType::fromJobName)
-                    .orElse(JobType.component);
+        controller.applications().lockedOrThrow(applicationId, application -> {
             // Since this is a manual operation we likely want it to trigger as soon as possible so we add it at to the
             // front of the queue
             application = controller.applications().deploymentTrigger().triggerAllowParallel(
@@ -117,12 +117,12 @@ public class ScrewdriverApiHandler extends LoggingRequestHandler {
                     "Triggered from screwdriver/v1"
             );
             controller.applications().store(application);
+        });
 
-            Slime slime = new Slime();
-            Cursor cursor = slime.setObject();
-            cursor.setString("message", "Triggered " + jobType.jobName() + " for " + applicationId);
-            return new SlimeJsonResponse(slime);
-        }
+        Slime slime = new Slime();
+        Cursor cursor = slime.setObject();
+        cursor.setString("message", "Triggered " + jobType.jobName() + " for " + applicationId);
+        return new SlimeJsonResponse(slime);
     }
 
     private HttpResponse vespaVersion() {
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 0738f9a7c00..d0c1fd95427 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
@@ -552,11 +552,7 @@ public class ControllerTest {
         // Load test data data
         ApplicationSerializer serializer = new ApplicationSerializer();
         byte[] json = Files.readAllBytes(Paths.get("src/test/java/com/yahoo/vespa/hosted/controller/maintenance/testdata/canary-with-stale-data.json"));
-        Slime slime = SlimeUtils.jsonToSlime(json);
-        Application application = serializer.fromSlime(slime);
-        try (Lock lock = tester.controller().applications().lock(application.id())) {
-            tester.controller().applications().store(new LockedApplication(application, lock));
-        }
+        Application application = tester.controllerTester().createApplication(SlimeUtils.jsonToSlime(json));
 
         ApplicationPackage applicationPackage = new ApplicationPackageBuilder()
                 .upgradePolicy("canary")
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
index 4e3c15ea1a4..8f9c22f8b81 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/ControllerTester.java
@@ -181,12 +181,9 @@ public final class ControllerTester {
     public Application createApplication(TenantId tenant, String applicationName, String instanceName, long projectId) {
         ApplicationId applicationId = applicationId(tenant.id(), applicationName, instanceName);
         controller().applications().createApplication(applicationId, Optional.of(TestIdentities.userNToken));
-        try (Lock lock = controller().applications().lock(applicationId)) {
-            LockedApplication lockedApplication = controller().applications().require(applicationId, lock)
-                    .withProjectId(projectId);
-            controller().applications().store(lockedApplication);
-            return lockedApplication;
-        }
+        controller().applications().lockedOrThrow(applicationId, lockedApplication ->
+                controller().applications().store(lockedApplication.withProjectId(projectId)));
+        return controller().applications().require(applicationId);
     }
 
     public void deploy(Application application, Zone zone) {
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index fd23ee76b73..1638cbb2e95 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -306,11 +306,10 @@ public class ApplicationApiTest extends ControllerContainerTest {
     }
 
     private void addIssues(ContainerControllerTester tester, ApplicationId id) {
-        try (Lock lock = tester.controller().applications().lock(id)) {
-            tester.controller().applications().store(tester.controller().applications().require(id, lock)
-                                                    .withDeploymentIssueId(IssueId.from("123"))
-                                                    .withOwnershipIssueId(IssueId.from("321")));
-        }
+        tester.controller().applications().lockedOrThrow(id, application ->
+                tester.controller().applications().store(application
+                                                                 .withDeploymentIssueId(IssueId.from("123"))
+                                                                 .withOwnershipIssueId(IssueId.from("321"))));
     }
 
     @Test
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
index ee8ac5e3b8e..e6b3eacd44e 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/screwdriver/ScrewdriverApiTest.java
@@ -148,11 +148,8 @@ public class ScrewdriverApiTest extends ControllerContainerTest {
         tester.containerTester().updateSystemVersion();
 
         Application app = tester.createApplication();
-        try (Lock lock = tester.controller().applications().lock(app.id())) {
-            tester.controller().applications().store(
-                    tester.controller().applications().require(app.id(), lock).withProjectId(1)
-            );
-        }
+        tester.controller().applications().lockedOrThrow(app.id(), application ->
+                tester.controller().applications().store(application.withProjectId(1)));
 
         // Unknown application
         assertResponse(new Request("http://localhost:8080/screwdriver/v1/trigger/tenant/foo/application/bar",