diff options
author | Martin Polden <mpolden@mpolden.no> | 2022-04-12 12:36:43 +0200 |
---|---|---|
committer | Martin Polden <mpolden@mpolden.no> | 2022-04-12 12:40:41 +0200 |
commit | 20fb5a698573eef66c6c76df5ca4fd2185dc80ec (patch) | |
tree | af3b2c9c2b9586fa90abbc1e69da13dc225d97b6 /node-repository/src/main/java/com | |
parent | 589e3662f610811e0e54992c6cc0e30d05c8630b (diff) |
Fix after review feedback
Diffstat (limited to 'node-repository/src/main/java/com')
10 files changed, 37 insertions, 38 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); |