diff options
author | Martin Polden <mpolden@mpolden.no> | 2019-02-15 08:57:02 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-02-15 08:57:02 +0100 |
commit | d96edb431241e3132f2bd23d5e673763e319fa63 (patch) | |
tree | 3a48a3f15b56792a635edeb7523d97d09ffa3a29 | |
parent | 7044a31a3576a5790dd5c2f0be1c126ce70882c7 (diff) | |
parent | b25652c01a51006e904592062e7f94bbf5fe1f16 (diff) |
Merge pull request #8505 from vespa-engine/hakonhall/remove-nodebuilder
Remove Node.Builder
7 files changed, 48 insertions, 169 deletions
diff --git a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGeneratorTest.java b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGeneratorTest.java index db600f78271..5727f35ae78 100644 --- a/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGeneratorTest.java +++ b/athenz-identity-provider-service/src/test/java/com/yahoo/vespa/hosted/athenz/instanceproviderservice/identitydocument/IdentityDocumentGeneratorTest.java @@ -66,7 +66,7 @@ public class IdentityDocumentGeneratorTest { Node containerNode = Node.createDockerNode(ImmutableSet.of("::1"), new HashSet<>(), containerHostname, - parentHostname, + Optional.of(parentHostname), new MockNodeFlavors().getFlavorOrThrow("default"), NodeType.tenant) .with(allocation); 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( |