Append, rather than set, logs, and fix unstable unit test

4 files changed, 20 insertions, 25 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/LogStore.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/LogStore.java
index ef2d9077892..23da48b4aad 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/LogStore.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/LogStore.java
@@ -11,12 +11,12 @@ import java.util.Optional;
 public interface LogStore {
 
     /** @return the log of the given step of the given deployment job, or an empty byte array if non-existent. */
-    byte[] getLog(RunId id, String step);
+    byte[] get(RunId id, String step);
 
     /** Stores the given log for the given step of the given deployment job. */
-    void setLog(RunId id, String step, byte[] log);
+    void append(RunId id, String step, byte[] log);
 
     /** Deletes all data associated with the given deployment job */
-    void deleteTestData(RunId id);
+    void delete(RunId id);
 
 }
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockLogStore.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockLogStore.java
index 61e6ac1004b..330b967c1b5 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockLogStore.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/stubs/MockLogStore.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.controller.api.integration.stubs;
 import com.yahoo.vespa.hosted.controller.api.integration.LogStore;
 import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId;
 
+import java.util.Collections;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
@@ -12,24 +13,26 @@ import java.util.concurrent.ConcurrentHashMap;
  */
 public class MockLogStore implements LogStore {
 
-    private final Map<RunId, Map<String, byte[]>> storage = new ConcurrentHashMap<>();
+    private final Map<RunId, Map<String, byte[]>> logs = new ConcurrentHashMap<>();
 
     @Override
-    public byte[] getLog(RunId id, String step) {
-        return storage.containsKey(id) && storage.get(id).containsKey(step)
-                ? storage.get(id).get(step)
-                : new byte[0];
+    public byte[] get(RunId id, String step) {
+        return logs.getOrDefault(id, Collections.emptyMap()).getOrDefault(step, new byte[0]);
     }
 
     @Override
-    public void setLog(RunId id, String step, byte[] log) {
-        storage.putIfAbsent(id, new ConcurrentHashMap<>());
-        storage.get(id).put(step, log);
+    public void append(RunId id, String step, byte[] log) {
+        logs.putIfAbsent(id, new ConcurrentHashMap<>());
+        byte[] old = get(id, step);
+        byte[] union = new byte[old.length + log.length];
+        System.arraycopy(old, 0, union, 0, old.length);
+        System.arraycopy(log, 0, union, old.length, log.length);
+        logs.get(id).put(step, union);
     }
 
     @Override
-    public void deleteTestData(RunId id) {
-        storage.remove(id);
+    public void delete(RunId id) {
+        logs.remove(id);
     }
 
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
index 056dcba2cd5..29895066525 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/JobController.java
@@ -16,7 +16,6 @@ import com.yahoo.vespa.hosted.controller.application.JobStatus;
 import com.yahoo.vespa.hosted.controller.application.SourceRevision;
 import com.yahoo.vespa.hosted.controller.persistence.CuratorDb;
 
-import java.io.ByteArrayInputStream;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -77,7 +76,7 @@ public class JobController {
 
         Map<Step, byte[]> details = new HashMap<>();
         for (Step step : run.steps().keySet()) {
-            byte[] log = logs.getLog(id, step.name());
+            byte[] log = logs.get(id, step.name());
             if (log.length > 0)
                 details.put(step, log);
         }
@@ -87,14 +86,7 @@ public class JobController {
     /** Appends the given log bytes to the currently stored bytes for the given run and step. */
     public void log(RunId id, Step step, byte[] log) {
         try (Lock __ = curator.lock(id.application(), id.type())) {
-            byte[] stored = logs.getLog(id, step.name());
-            if (stored.length > 0) {
-                byte[] addition = log;
-                log = new byte[stored.length + addition.length];
-                System.arraycopy(stored, 0, log, 0, stored.length);
-                System.arraycopy(addition, 0, log, stored.length, addition.length);
-            }
-            logs.setLog(id, step.name(), log);
+            logs.append(id, step.name(), log);
         }
     }
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
index 49cfa129249..a1436b61203 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java
@@ -85,7 +85,7 @@ public class JobRunnerTest {
         latch.await(1, TimeUnit.SECONDS);
         assertEquals(0, latch.getCount());
 
-        jobs.updateStorage(); // Holding the lock of each job ensures data read below is fresh.
+        runner.deconstruct(); // Ensures all workers have finished writing to the curator.
         assertTrue(jobs.last(id, systemTest).get().steps().values().stream().allMatch(succeeded::equals));
         assertTrue(jobs.last(id, stagingTest).get().hasEnded());
         assertTrue(jobs.last(id, stagingTest).get().hasFailed());
@@ -170,7 +170,7 @@ public class JobRunnerTest {
     }
 
     @Test
-    public void stepLocking() throws InterruptedException, BrokenBarrierException {
+    public void locksAndGarbage() throws InterruptedException, BrokenBarrierException {
         DeploymentTester tester = new DeploymentTester();
         JobController jobs = tester.controller().jobController();
         // Hang during tester deployment, until notified.