Merge pull request #8505 from vespa-engine/hakonhall/remove-nodebuilder

Remove Node.Builder

6 files changed, 47 insertions, 168 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 a52ffb11fb7..c0ddfea536d 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
@@ -15,7 +15,6 @@ import com.yahoo.vespa.hosted.provision.node.Reports;
 import com.yahoo.vespa.hosted.provision.node.Status;
 
 import java.time.Instant;
-import java.util.Collections;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
@@ -49,27 +48,24 @@ public final class Node {
     private Optional<Allocation> allocation;
 
     /** Temporary method until we can merge it with the other create method */
-    public static Node createDockerNode(Set<String> ipAddresses, Set<String> ipAddressPool, String hostname, String parentHostname, Flavor flavor, NodeType type) {
-        return new Builder("fake-" + hostname, ipAddresses, hostname, flavor, type)
-                .withIpAddressPool(ipAddressPool)
-                .withParentHostname(parentHostname)
-                .withState(State.reserved)
-                .build();
+    public static Node createDockerNode(Set<String> ipAddresses, Set<String> ipAddressPool, String hostname, Optional<String> parentHostname, Flavor flavor, NodeType type) {
+        return new Node("fake-" + hostname, ipAddresses, ipAddressPool, hostname, parentHostname, flavor, Status.initial(), State.reserved,
+                Optional.empty(), History.empty(), type, new Reports(), Optional.empty());
     }
 
     /** Creates a node in the initial state (provisioned) */
     public static Node create(String openStackId, Set<String> ipAddresses, Set<String> ipAddressPool, String hostname, Optional<String> parentHostname, Flavor flavor, NodeType type) {
-        return new Builder(openStackId, ipAddresses, hostname, flavor, type)
-                .withIpAddressPool(ipAddressPool)
-                .withParentHostname(parentHostname)
-                .build();
+        return new Node(openStackId, ipAddresses, ipAddressPool, hostname, parentHostname, flavor, Status.initial(), State.provisioned,
+                Optional.empty(), History.empty(), type, new Reports(), Optional.empty());
     }
 
-    /**
-     * Creates a node with all fields specified, necessary for serialization code.
-     *
-     * Others should use the {@code create} methods to create nodes, or the Builder to modify nodes.
-     */
+    /** Do not use. Construct nodes by calling {@link NodeRepository#createNode} */
+    private Node(String id, Set<String> ipAddresses, Set<String> ipAddressPool, String hostname, Optional<String> parentHostname,
+                 Flavor flavor, Status status, State state, Allocation allocation, History history, NodeType type) {
+        this(id, ipAddresses, ipAddressPool, hostname, parentHostname, flavor, status, state, Optional.of(allocation), history, type, new Reports(), Optional.empty());
+    }
+
+    /** Creates a node. See also the {@code create} helper methods. */
     public Node(String id, Set<String> ipAddresses, Set<String> ipAddressPool, String hostname, Optional<String> parentHostname,
                 Flavor flavor, Status status, State state, Optional<Allocation> allocation, History history, NodeType type,
                 Reports reports, Optional<String> modelId) {
@@ -103,119 +99,6 @@ public final class Node {
         this.modelId = modelId;
     }
 
-    /** Helper for creating and mutating node objects. */
-    private static class Builder {
-        private final String hostname;
-
-        // Required but mutable fields
-        private String id;
-        private NodeType type;
-        private Flavor flavor;
-        private Set<String> ipAddresses;
-
-        private Optional<String> modelId = Optional.empty();
-        private Set<String> ipAddressPool = Collections.emptySet();
-        private Optional<String> parentHostname = Optional.empty();
-        private Status status = Status.initial();
-        private State state = State.provisioned;
-        private History history = History.empty();
-        private Optional<Allocation> allocation = Optional.empty();
-        private Reports reports = new Reports();
-
-        /** Creates a builder fairly well suited for a newly provisioned node (but see {@code create} and {@code createDockerNode}). */
-        private Builder(String id, Set<String> ipAddresses, String hostname, Flavor flavor, NodeType type) {
-            this.id = id;
-            this.ipAddresses = ipAddresses;
-            this.hostname = hostname;
-            this.flavor = flavor;
-            this.type = type;
-        }
-
-        private Builder(Node node) {
-            this(node.id, node.ipAddresses, node.hostname, node.flavor, node.type);
-            withIpAddressPool(node.ipAddressPool.asSet());
-            node.parentHostname.ifPresent(this::withParentHostname);
-            withStatus(node.status);
-            withState(node.state);
-            withHistory(node.history);
-            node.allocation.ifPresent(this::withAllocation);
-        }
-
-        public Builder withId(String id) {
-            this.id = id;
-            return this;
-        }
-
-        public Builder withModelId(Optional<String> modelId) {
-            this.modelId = modelId;
-            return this;
-        }
-
-        public Builder withType(NodeType nodeType) {
-            this.type = nodeType;
-            return this;
-        }
-
-        public Builder withFlavor(Flavor flavor) {
-            this.flavor = flavor;
-            return this;
-        }
-
-        public Builder withIpAddresses(Set<String> ipAddresses) {
-            this.ipAddresses = ipAddresses;
-            return this;
-        }
-
-        public Builder withIpAddressPool(Set<String> ipAddressPool) {
-            this.ipAddressPool = ipAddressPool;
-            return this;
-        }
-
-        public Builder withParentHostname(String parentHostname) {
-            this.parentHostname = Optional.of(parentHostname);
-            return this;
-        }
-
-        public Builder withParentHostname(Optional<String> parentHostname) {
-            parentHostname.ifPresent(this::withParentHostname);
-            return this;
-        }
-
-        public Builder withStatus(Status status) {
-            this.status = status;
-            return this;
-        }
-
-        public Builder withState(State state) {
-            this.state = state;
-            return this;
-        }
-
-        public Builder withHistory(History history) {
-            this.history = history;
-            return this;
-        }
-
-        public Builder withHistoryEvent(History.Event.Type type, Agent agent, Instant at) {
-            return withHistory(history.with(new History.Event(type, agent, at)));
-        }
-
-        public Builder withAllocation(Allocation allocation) {
-            this.allocation = Optional.of(allocation);
-            return this;
-        }
-
-        public Builder withReports(Reports reports) {
-            this.reports = reports;
-            return this;
-        }
-
-        public Node build() {
-            return new Node(id, ipAddresses, ipAddressPool, hostname, parentHostname, flavor, status,
-                    state, allocation, history, type, reports, modelId);
-        }
-    }
-
     /** Returns the IP addresses of this node */
     public Set<String> ipAddresses() { return ipAddresses; }
 
@@ -280,12 +163,8 @@ public final class Node {
      */
     public Node withWantToRetire(boolean wantToRetire, Agent agent, Instant at) {
         if (wantToRetire == status.wantToRetire()) return this;
-        return new Builder(this)
-                .withStatus(status.withWantToRetire(wantToRetire))
-                // Also update history when we un-wantToRetire so the OperatorChangeApplicationMaintainer picks it
-                // up quickly
-                .withHistoryEvent(History.Event.Type.wantToRetire, agent, at)
-                .build();
+        return with(status.withWantToRetire(wantToRetire))
+                .with(history.with(new History.Event(History.Event.Type.wantToRetire, Agent.operator, at)));
     }
 
     /**
@@ -295,10 +174,8 @@ public final class Node {
     public Node retire(Agent agent, Instant retiredAt) {
         Allocation allocation = requireAllocation("Cannot retire");
         if (allocation.membership().retired()) return this;
-        return new Builder(this)
-                .withAllocation(allocation.retire())
-                .withHistoryEvent(History.Event.Type.retired, agent, retiredAt)
-                .build();
+        return with(allocation.retire())
+                .with(history.with(new History.Event(History.Event.Type.retired, agent, retiredAt)));
     }
 
     /** Returns a copy of this node which is retired */
@@ -317,54 +194,52 @@ public final class Node {
     /** Returns a copy of this with the restart generation set to generation */
     public Node withRestart(Generation generation) {
         Allocation allocation = requireAllocation("Cannot set restart generation");
-        return new Builder(this).withAllocation(allocation.withRestart(generation)).build();
+        return with(allocation.withRestart(generation));
     }
 
     /** Returns a node with the status assigned to the given value */
     public Node with(Status status) {
-        return new Builder(this).withStatus(status).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     /** Returns a node with the type assigned to the given value */
     public Node with(NodeType type) {
-        return new Builder(this).withType(type).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     /** Returns a node with the flavor assigned to the given value */
     public Node with(Flavor flavor) {
-        return new Builder(this).withFlavor(flavor).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     /** Returns a copy of this with the reboot generation set to generation */
     public Node withReboot(Generation generation) {
-        return new Builder(this).withStatus(status.withReboot(generation)).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status.withReboot(generation), state, allocation, history, type, reports, modelId);
     }
 
     /** Returns a copy of this with the openStackId set */
     public Node withOpenStackId(String openStackId) {
-        return new Builder(this).withId(openStackId).build();
+        return new Node(openStackId, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     public Node withModelId(String modelId) {
-        return new Builder(this).withModelId(Optional.of(modelId)).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, Optional.of(modelId));
     }
 
     /** Returns a copy of this with a history record saying it was detected to be down at this instant */
     public Node downAt(Instant instant) {
-        return new Builder(this).withHistoryEvent(History.Event.Type.down, Agent.system, instant).build();
+        return with(history.with(new History.Event(History.Event.Type.down, Agent.system, instant)));
     }
 
     /** Returns a copy of this with any history record saying it has been detected down removed */
     public Node up() {
-        return new Builder(this).withHistory(history.without(History.Event.Type.down)).build();
+        return with(history.without(History.Event.Type.down));
     }
 
     /** Returns a copy of this with allocation set as specified. <code>node.state</code> is *not* changed. */
     public Node allocate(ApplicationId owner, ClusterMembership membership, Instant at) {
-        return new Builder(this)
-                .withAllocation(new Allocation(owner, membership, new Generation(0, 0), false))
-                .withHistoryEvent(History.Event.Type.reserved, Agent.application, at)
-                .build();
+        return this.with(new Allocation(owner, membership, new Generation(0, 0), false))
+                .with(history.with(new History.Event(History.Event.Type.reserved, Agent.application, at)));
     }
 
     /**
@@ -372,40 +247,43 @@ public final class Node {
      * Do not use this to allocate a node.
      */
     public Node with(Allocation allocation) {
-        return new Builder(this).withAllocation(allocation).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type);
     }
 
     /** Returns a copy of this node with the IP addresses set to the given value. */
     public Node withIpAddresses(Set<String> ipAddresses) {
-        return new Builder(this).withIpAddresses(ipAddresses).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state,
+                allocation, history, type, reports, modelId);
     }
 
     /** Returns a copy of this node with IP address pool set to the given value. */
     public Node withIpAddressPool(Set<String> ipAddressPool) {
-        return new Builder(this).withIpAddressPool(ipAddressPool).build();
+        return new Node(id, ipAddresses, ipAddressPool, hostname, parentHostname, flavor, status, state,
+                allocation, history, type, reports, modelId);
     }
 
     /** Returns a copy of this node with the parent hostname assigned to the given value. */
     public Node withParentHostname(String parentHostname) {
-        return new Builder(this).withParentHostname(parentHostname).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, Optional.of(parentHostname), flavor, status, state,
+                allocation, history, type, reports, modelId);
     }
 
     /** Returns a copy of this node with the current reboot generation set to the given number at the given instant */
     public Node withCurrentRebootGeneration(long generation, Instant instant) {
-        Builder builder = new Builder(this)
-                .withStatus(status().withReboot(status().reboot().withCurrent(generation)));
+        Status newStatus = status().withReboot(status().reboot().withCurrent(generation));
+        History newHistory = history();
         if (generation > status().reboot().current())
-            builder.withHistoryEvent(History.Event.Type.rebooted, Agent.system, instant);
-        return builder.build();
+            newHistory = history.with(new History.Event(History.Event.Type.rebooted, Agent.system, instant));
+        return this.with(newStatus).with(newHistory);
     }
 
     /** Returns a copy of this node with the given history. */
     public Node with(History history) {
-        return new Builder(this).withHistory(history).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     public Node with(Reports reports) {
-        return new Builder(this).withReports(reports).build();
+        return new Node(id, ipAddresses, ipAddressPool.asSet(), hostname, parentHostname, flavor, status, state, allocation, history, type, reports, modelId);
     }
 
     private static void requireNonEmptyString(Optional<String> value, String message) {
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
index b88ed8c124f..861e1062d66 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizer.java
@@ -165,7 +165,7 @@ class NodePrioritizer {
             Node newNode = Node.createDockerNode(allocation.get().addresses(),
                                                  Collections.emptySet(),
                                                  allocation.get().hostname(),
-                                                 node.hostname(),
+                                                 Optional.of(node.hostname()),
                                                  getFlavor(requestedNodes),
                                                  NodeType.tenant);
             PrioritizableNode nodePri = toNodePriority(newNode, false, true);
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionedHost.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionedHost.java
index eb10a194434..d13643de5b1 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionedHost.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/ProvisionedHost.java
@@ -36,7 +36,7 @@ public class ProvisionedHost {
 
     /** Generate {@link Node} instance representing the node running on this physical host */
     Node generateNode() {
-        return Node.createDockerNode(Set.of(), Set.of(), nodeHostname, hostHostname, nodeFlavor, NodeType.tenant);
+        return Node.createDockerNode(Set.of(), Set.of(), nodeHostname, Optional.of(hostHostname), nodeFlavor, NodeType.tenant);
     }
 
     @Override
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java
index 57b942d85e4..cc7ad26f1e0 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/monitoring/MetricsReporterTest.java
@@ -126,11 +126,11 @@ public class MetricsReporterTest {
         nodeRepository.dirtyRecursively("dockerHost", Agent.system, getClass().getSimpleName());
         nodeRepository.setReady("dockerHost", Agent.system, getClass().getSimpleName());
 
-        Node container1 = Node.createDockerNode(Collections.singleton("::2"), Collections.emptySet(), "container1", "dockerHost", nodeFlavors.getFlavorOrThrow("docker"), NodeType.tenant);
+        Node container1 = Node.createDockerNode(Collections.singleton("::2"), Collections.emptySet(), "container1", Optional.of("dockerHost"), nodeFlavors.getFlavorOrThrow("docker"), NodeType.tenant);
         container1 = container1.with(allocation(Optional.of("app1")).get());
         nodeRepository.addDockerNodes(Collections.singletonList(container1), nodeRepository.lockAllocation());
 
-        Node container2 = Node.createDockerNode(Collections.singleton("::3"), Collections.emptySet(), "container2", "dockerHost", nodeFlavors.getFlavorOrThrow("docker2"), NodeType.tenant);
+        Node container2 = Node.createDockerNode(Collections.singleton("::3"), Collections.emptySet(), "container2", Optional.of("dockerHost"), nodeFlavors.getFlavorOrThrow("docker2"), NodeType.tenant);
         container2 = container2.with(allocation(Optional.of("app2")).get());
         nodeRepository.addDockerNodes(Collections.singletonList(container2), nodeRepository.lockAllocation());
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizerTest.java
index 85dc97e5aba..0ba14d3fb67 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizerTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/provisioning/NodePrioritizerTest.java
@@ -68,7 +68,7 @@ public class NodePrioritizerTest {
     }
 
     private static Node createNode(Node parent, String hostname, String flavor) {
-        return Node.createDockerNode(Collections.singleton("127.0.0.1"), new HashSet<>(), hostname, parent.hostname(),
+        return Node.createDockerNode(Collections.singleton("127.0.0.1"), new HashSet<>(), hostname, Optional.of(parent.hostname()),
                                      flavors.getFlavorOrThrow(flavor), NodeType.tenant);
     }
 
diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java
index 51c32c8f85a..cc23be4ddb4 100644
--- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java
+++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodeIdentifierTest.java
@@ -32,6 +32,7 @@ import java.security.KeyPair;
 import java.security.cert.X509Certificate;
 import java.time.Instant;
 import java.util.Collections;
+import java.util.Optional;
 
 import static com.yahoo.security.KeyAlgorithm.EC;
 import static com.yahoo.security.SignatureAlgorithm.SHA256_WITH_ECDSA;
@@ -232,7 +233,7 @@ public class NodeIdentifierTest {
                         singleton("1.2.3.4"),
                         emptySet(),
                         HOSTNAME,
-                        "parenthost",
+                        Optional.of("parenthost"),
                         new Flavor(createFlavourConfig().flavor(0)),
                         NodeType.tenant)
                 .with(