Fix after review feedback

14 files changed, 47 insertions, 64 deletions

diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
index 783189d5bbf..853c67dc169 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java
@@ -359,10 +359,10 @@ public final class Node implements Nodelike {
 
     /** Returns whether this node is down, according to its recorded 'down' and 'up' events */
     public boolean isDown() {
-        Optional<Instant> downAt = history().event(History.Event.Type.down).map(History.Event::at);
+        Optional<Instant> downAt = history().lastEvent(History.Event.Type.down).map(History.Event::at);
         if (downAt.isEmpty()) return false;
 
-        Optional<Instant> upAt = history().event(History.Event.Type.up).map(History.Event::at);
+        Optional<Instant> upAt = history().lastEvent(History.Event.Type.up).map(History.Event::at);
         if (upAt.isEmpty()) return true;
         return !downAt.get().isBefore(upAt.get());
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
index eab9f755db2..45a90daac96 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/AutoscalingMaintainer.java
@@ -107,7 +107,7 @@ public class AutoscalingMaintainer extends NodeRepositoryMaintainer {
         // Scaling event is complete if:
         // - 1. no nodes which was retired by this are still present (which also implies data distribution is complete)
         if (clusterNodes.retired().stream()
-                        .anyMatch(node -> node.history().hasEventAt(History.Event.Type.retired, event.at())))
+                        .anyMatch(node -> node.history().hasLastEventAt(event.at(), History.Event.Type.retired)))
             return cluster;
         // - 2. all nodes have switched to the right config generation (currently only measured on containers)
         for (var nodeTimeseries : nodeRepository().metricsDb().getNodeTimeseries(Duration.between(event.at(), clock().instant()),
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Expirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Expirer.java
index 5a77fcca85c..51d73bd8a58 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Expirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/Expirer.java
@@ -57,7 +57,7 @@ public abstract class Expirer extends NodeRepositoryMaintainer {
     }
 
     protected final boolean isExpired(Node node, Duration expiryTime) {
-        return node.history().hasEventBefore(eventType, clock().instant().minus(expiryTime));
+        return node.history().hasLastEventBefore(clock().instant().minus(expiryTime), eventType);
     }
 
     /** Implement this callback to take action to expire these nodes */
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
index 3274f12dbc6..af17260ad84 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/FailedExpirer.java
@@ -73,10 +73,10 @@ public class FailedExpirer extends NodeRepositoryMaintainer {
         recycleIf(remainingNodes, node -> node.allocation().isEmpty());
         recycleIf(remainingNodes, node ->
                 !node.allocation().get().membership().cluster().isStateful() &&
-                node.history().hasEventBefore(History.Event.Type.failed, clock().instant().minus(statelessExpiry)));
+                node.history().hasLastEventBefore(clock().instant().minus(statelessExpiry), History.Event.Type.failed));
         recycleIf(remainingNodes, node ->
                 node.allocation().get().membership().cluster().isStateful() &&
-                node.history().hasEventBefore(History.Event.Type.failed, clock().instant().minus(statefulExpiry)));
+                node.history().hasLastEventBefore(clock().instant().minus(statefulExpiry), History.Event.Type.failed));
         return 1.0;
     }
 
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
index 1f6862a1dc4..661ba531de5 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeFailer.java
@@ -157,7 +157,7 @@ public class NodeFailer extends NodeRepositoryMaintainer {
             Instant graceTimeStart = clock().instant().minus(nodeRepository().nodes().suspended(node) ? suspendedDownTimeLimit : downTimeLimit);
             if (downBefore(graceTimeStart, node) && !applicationSuspended(node)) {
                 // Allow a grace period after node re-activation
-                if (!node.history().hasEventAfter(History.Event.Type.activated, graceTimeStart))
+                if (!node.history().hasLastEventAfter(graceTimeStart, History.Event.Type.activated))
                     failingNodes.add(new FailingNode(node, "Node has been down longer than " + downTimeLimit));
             }
         }
@@ -280,7 +280,7 @@ public class NodeFailer extends NodeRepositoryMaintainer {
 
     /** Returns whether node is down, and has been down since before given instant */
     private static boolean downBefore(Instant instant, Node node) {
-        return node.isDown() && node.history().event(History.Event.Type.down).get().at().isBefore(instant);
+        return node.isDown() && node.history().hasLastEventBefore(instant, History.Event.Type.down);
     }
 
     private void wantToFail(Node node, boolean wantToFail, Mutex lock) {
@@ -293,8 +293,8 @@ public class NodeFailer extends NodeRepositoryMaintainer {
         Instant startOfThrottleWindow = clock().instant().minus(throttlePolicy.throttleWindow);
         NodeList allNodes = nodeRepository().nodes().list();
         NodeList recentlyFailedNodes = allNodes.state(Node.State.failed)
-                                               .matching(n -> n.history().hasEventAfter(History.Event.Type.failed,
-                                                                                        startOfThrottleWindow));
+                                               .matching(n -> n.history().hasLastEventAfter(startOfThrottleWindow, History.Event.Type.failed
+                                               ));
 
         // Allow failing any node within policy
         if (recentlyFailedNodes.size() < throttlePolicy.allowedToFailOf(allNodes.size())) return false;
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeHealthTracker.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeHealthTracker.java
index 1eb651598e8..ab2cc2da257 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeHealthTracker.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/NodeHealthTracker.java
@@ -61,7 +61,7 @@ public class NodeHealthTracker extends NodeRepositoryMaintainer {
                 Optional<Instant> lastLocalRequest = hostLivenessTracker.lastRequestFrom(node.hostname());
                 if (lastLocalRequest.isEmpty()) continue;
 
-                if (!node.history().hasEventAfter(History.Event.Type.requested, lastLocalRequest.get())) {
+                if (!node.history().hasLastEventAfter(lastLocalRequest.get(), History.Event.Type.requested)) {
                     History updatedHistory = node.history()
                                                  .with(new History.Event(History.Event.Type.requested, Agent.NodeHealthTracker, lastLocalRequest.get()));
                     nodeRepository().nodes().write(node.with(updatedHistory), lock);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
index 2db02992f40..39015306ae2 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/ProvisionedExpirer.java
@@ -54,7 +54,7 @@ public class ProvisionedExpirer extends Expirer {
     }
 
     private boolean parkedByProvisionedExpirer(Node node) {
-        return node.history().event(History.Event.Type.parked)
+        return node.history().lastEvent(History.Event.Type.parked)
                 .map(History.Event::agent)
                 .map(Agent.ProvisionedExpirer::equals)
                 .orElse(false);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
index 73c9a1ab55a..d4eb02b59b0 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/RetiredExpirer.java
@@ -116,7 +116,7 @@ public class RetiredExpirer extends NodeRepositoryMaintainer {
                 // allowing the removal of any config server.
                 return false;
             }
-        } else if (node.history().hasEventBefore(History.Event.Type.retired, clock().instant().minus(retiredExpiry))) {
+        } else if (node.history().hasLastEventBefore(clock().instant().minus(retiredExpiry), History.Event.Type.retired)) {
             log.warning("Node " + node + " has been retired longer than " + retiredExpiry + ": Allowing removal. This may cause data loss");
             return true;
         }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
index 2515e570dbb..59e43fd9971 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/History.java
@@ -38,27 +38,27 @@ public class History {
                              .collect(Collectors.toUnmodifiableList());
     }
 
-    /** Returns the latest event of given type, if it is present in this history */
-    public Optional<Event> event(Event.Type type) {
+    /** Returns the last event of given type, if it is present in this history */
+    public Optional<Event> lastEvent(Event.Type type) {
         return events.stream().filter(event -> event.type() == type).max(Comparator.comparing(Event::at));
     }
 
-    /** Returns true if a given event is registered in this history at the given time */
-    public boolean hasEventAt(Event.Type type, Instant time) {
-        return event(type).map(event -> event.at().equals(time))
-                          .orElse(false);
+    /** Returns true if the last event of this type is registered in this history at the given time */
+    public boolean hasLastEventAt(Instant time, Event.Type type) {
+        return lastEvent(type).map(event -> event.at().equals(time))
+                              .orElse(false);
     }
 
-    /** Returns true if a given event is registered in this history after the given time */
-    public boolean hasEventAfter(Event.Type type, Instant time) {
-        return event(type).map(event -> event.at().isAfter(time))
-                          .orElse(false);
+    /** Returns true if the last event of this type is registered after the given time */
+    public boolean hasLastEventAfter(Instant time, Event.Type type) {
+        return lastEvent(type).map(event -> event.at().isAfter(time))
+                              .orElse(false);
     }
 
-    /** Returns true if a given event is registered in this history before the given time */
-    public boolean hasEventBefore(Event.Type type, Instant time) {
-        return event(type).map(event -> event.at().isBefore(time))
-                          .orElse(false);
+    /** Returns true if the last event of this type is registered before the given time */
+    public boolean hasLastEventBefore(Instant time, Event.Type type) {
+        return lastEvent(type).map(event -> event.at().isBefore(time))
+                              .orElse(false);
     }
 
     public List<Event> asList() {
@@ -68,21 +68,13 @@ public class History {
     /** Returns a copy of this history with the given event added */
     public History with(Event event) {
         List<Event> copy = new ArrayList<>(events);
-        if (!copy.isEmpty()) {
-            // Let given event overwrite the latest if they're of the same type. Some events may be repeated, such as
-            // 'reserved'
-            Event last = copy.get(copy.size() - 1);
-            if (last.type() == event.type()) {
-                copy.remove(last);
-            }
-        }
         copy.add(event);
         return new History(copy);
     }
 
     /** Returns a copy of this history with a record of this state transition added, if applicable */
     public History recordStateTransition(Node.State from, Node.State to, Agent agent, Instant at) {
-        // If the event is a re-reservation, allow the new one to override the older one.
+        // If the event is a re-reservation, allow the new event to overwrite the older one.
         if (from == to && from != Node.State.reserved) return this;
         switch (to) {
             case provisioned:   return this.with(new Event(Event.Type.provisioned, agent, at));
@@ -90,7 +82,14 @@ public class History {
             case ready:         return this.withoutApplicationEvents().with(new Event(Event.Type.readied, agent, at));
             case active:        return this.with(new Event(Event.Type.activated, agent, at));
             case inactive:      return this.with(new Event(Event.Type.deactivated, agent, at));
-            case reserved:      return this.with(new Event(Event.Type.reserved, agent, at));
+            case reserved: {
+                History history = this;
+                if (!events.isEmpty() && events.get(events.size() - 1).type() == Event.Type.reserved) {
+                    // Avoid repeating reserved event
+                    history = new History(events.subList(0, events.size() - 1));
+                }
+                return history.with(new Event(Event.Type.reserved, agent, at));
+            }
             case failed:        return this.with(new Event(Event.Type.failed, agent, at));
             case dirty:         return this.with(new Event(Event.Type.deallocated, agent, at));
             case parked:        return this.with(new Event(Event.Type.parked, agent, at));
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
index d544ea76983..c403dda1b74 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java
@@ -840,7 +840,7 @@ public class Nodes {
         if (agent == Agent.operator) return false;
         if (node.type() == NodeType.tenant && node.status().wantToDeprovision()) return false;
         boolean retirementRequestedByOperator = node.status().wantToRetire() &&
-                                                node.history().event(History.Event.Type.wantToRetire)
+                                                node.history().lastEvent(History.Event.Type.wantToRetire)
                                                     .map(History.Event::agent)
                                                     .map(a -> a == Agent.operator)
                                                     .orElse(false);
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
index 078df5264a1..74fe5800e89 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/NodeRepositoryTest.java
@@ -185,7 +185,7 @@ public class NodeRepositoryTest {
         tester.clock().advance(Duration.ofSeconds(1));
         tester.addHost("id1", "host1", "default", NodeType.host);
         tester.addHost("id2", "host2", "default", NodeType.host);
-        assertFalse(tester.nodeRepository().nodes().node("host1").get().history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
+        assertFalse(tester.nodeRepository().nodes().node("host1").get().history().hasLastEventAfter(testStart, History.Event.Type.deprovisioned));
 
         // Set host 1 properties and deprovision it
         try (var lock = tester.nodeRepository().nodes().lockAndGetRequired("host1")) {
@@ -208,7 +208,7 @@ public class NodeRepositoryTest {
         Node host1 = tester.nodeRepository().nodes().node("host1").get();
         Node host2 = tester.nodeRepository().nodes().node("host2").get();
         assertEquals(Node.State.deprovisioned, host1.state());
-        assertTrue(host1.history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
+        assertTrue(host1.history().hasLastEventAfter(testStart, History.Event.Type.deprovisioned));
 
         // Adding it again preserves some information from the deprovisioned host and removes it
         tester.addHost("id2", "host1", "default", NodeType.host);
@@ -218,7 +218,7 @@ public class NodeRepositoryTest {
                     tester.nodeRepository().nodes().node("host1", Node.State.deprovisioned).isPresent());
         assertFalse("Not transferred from deprovisioned host", host1.status().wantToRetire());
         assertFalse("Not transferred from deprovisioned host", host1.status().wantToDeprovision());
-        assertTrue("Transferred from deprovisioned host", host1.history().hasEventAfter(History.Event.Type.deprovisioned, testStart));
+        assertTrue("Transferred from deprovisioned host", host1.history().hasLastEventAfter(testStart, History.Event.Type.deprovisioned));
         assertTrue("Transferred from deprovisioned host", host1.status().firmwareVerifiedAt().isPresent());
         assertEquals("Transferred from deprovisioned host", 1, host1.status().failCount());
         assertEquals("Transferred from deprovisioned host", 1, host1.reports().getReports().size());
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/node/HistoryTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/node/HistoryTest.java
index 90162cc3ea3..62c5dda51a7 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/node/HistoryTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/node/HistoryTest.java
@@ -30,22 +30,6 @@ public class HistoryTest {
                      history.asList().stream().map(e -> e.at().toEpochMilli()).collect(Collectors.toList()));
     }
 
-    @Test
-    public void repeating_event_overwrites_existing() {
-        Instant i0 = Instant.ofEpochMilli(1);
-        History history = new History(List.of(new Event(Event.Type.readied, Agent.system, i0)));
-
-        Instant i1 = Instant.ofEpochMilli(2);
-        history = history.with(new Event(Event.Type.reserved, Agent.system, i1));
-        assertEquals(2, history.asList().size());
-
-        Instant i2 = Instant.ofEpochMilli(3);
-        history = history.with(new Event(Event.Type.reserved, Agent.system, i2));
-
-        assertEquals(2, history.asList().size());
-        assertEquals(i2, history.asList().get(1).at());
-    }
-
     private static List<Event> shuffledEvents(int count) {
         Instant start = Instant.ofEpochMilli(0);
         List<Event> events = new ArrayList<>();
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
index af3b5dc69f2..f14675d1536 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/persistence/NodeSerializerTest.java
@@ -113,7 +113,7 @@ public class NodeSerializerTest {
         assertEquals(node.allocation().get().requestedResources(), copy.allocation().get().requestedResources());
         assertEquals(node.allocation().get().isRemovable(), copy.allocation().get().isRemovable());
         assertEquals(2, copy.history().asList().size());
-        assertEquals(clock.instant().truncatedTo(MILLIS), copy.history().event(History.Event.Type.reserved).get().at());
+        assertEquals(clock.instant().truncatedTo(MILLIS), copy.history().lastEvent(History.Event.Type.reserved).get().at());
         assertEquals(NodeType.tenant, copy.type());
     }
 
@@ -182,9 +182,9 @@ public class NodeSerializerTest {
         node = node.retire(Agent.application, clock.instant());
         Node copy = nodeSerializer.fromJson(Node.State.provisioned, nodeSerializer.toJson(node));
         assertEquals(2, copy.history().asList().size());
-        assertEquals(clock.instant().truncatedTo(MILLIS), copy.history().event(History.Event.Type.retired).get().at());
+        assertEquals(clock.instant().truncatedTo(MILLIS), copy.history().lastEvent(History.Event.Type.retired).get().at());
         assertEquals(Agent.application,
-                     (copy.history().event(History.Event.Type.retired).get()).agent());
+                     (copy.history().lastEvent(History.Event.Type.retired).get()).agent());
         assertTrue(copy.allocation().get().membership().retired());
 
         Node removable = copy.with(node.allocation().get().removable(true));
@@ -296,7 +296,7 @@ public class NodeSerializerTest {
         Node copy = nodeSerializer.fromJson(Node.State.provisioned, nodeSerializer.toJson(node));
         assertEquals(1234, copy.flavor().resources().diskGb(), 0);
         assertEquals(node, copy);
-        assertTrue(node.history().event(History.Event.Type.resized).isPresent());
+        assertTrue(node.history().lastEvent(History.Event.Type.resized).isPresent());
     }
 
     @Test
@@ -372,7 +372,7 @@ public class NodeSerializerTest {
         node = node.withFirmwareVerifiedAt(Instant.ofEpochMilli(100));
         node = nodeSerializer.fromJson(State.active, nodeSerializer.toJson(node));
         assertEquals(100, node.status().firmwareVerifiedAt().get().toEpochMilli());
-        assertEquals(Instant.ofEpochMilli(100), node.history().event(History.Event.Type.firmwareVerified).get().at());
+        assertEquals(Instant.ofEpochMilli(100), node.history().lastEvent(History.Event.Type.firmwareVerified).get().at());
     }
 
     @Test
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
index 94822c85a03..a74eaf32bf8 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisioningTest.java
@@ -880,7 +880,7 @@ public class ProvisioningTest {
         assertTrue("Time of event is updated for all nodes",
                    reserved.stream()
                            .allMatch(n -> n.history()
-                           .event(History.Event.Type.reserved)
+                           .lastEvent(History.Event.Type.reserved)
                            .get().at()
                            .equals(tester.clock().instant().truncatedTo(MILLIS))));
 
@@ -1167,7 +1167,7 @@ public class ProvisioningTest {
 
     /** A predicate that returns whether a node has been retired by the given agent */
     private static Predicate<Node> retiredBy(Agent agent) {
-        return (node) -> node.history().event(History.Event.Type.retired)
+        return (node) -> node.history().lastEvent(History.Event.Type.retired)
                 .filter(e -> e.type() == History.Event.Type.retired)
                 .filter(e -> e.agent() == agent)
                 .isPresent();