diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java index 5dbd6ee54beb..0ce3a7a34395 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java @@ -43,9 +43,9 @@ *

* This class is value-based. */ -public final class NodeStatus implements Comparable { +public final class NodeStatus { /** For the {@link NodeStatus} objects with {@link #opStateExpiryEpochSeconds} == 0. */ - private static final Map> CONSTANT_MAP; + private static final Map> CONSTANTS; static { final Map> map = new EnumMap<>(NodeOperationalState.class); for (NodeOperationalState op : NodeOperationalState.values()) { @@ -53,19 +53,21 @@ public final class NodeStatus implements Comparable { for (NodeState health : NodeState.values()) { healthMap.put(health, new NodeStatus(health, op, 0)); } - map.put(op, healthMap); + map.put(op, Collections.unmodifiableMap(healthMap)); } - CONSTANT_MAP = map; + CONSTANTS = Collections.unmodifiableMap(map); } /** @return a {@link NodeStatus} object with {@link #opStateExpiryEpochSeconds} == 0. */ public static NodeStatus valueOf(NodeOperationalState op, NodeState health) { - return CONSTANT_MAP.get(op).get(health); + return valueOf(op, health, 0); } /** @return a {@link NodeStatus} object. */ public static NodeStatus valueOf(NodeOperationalState op, NodeState health, long opExpiryEpochSeconds) { - return opExpiryEpochSeconds == 0 ? valueOf(op, health) + Objects.requireNonNull(op, "op == null"); + Objects.requireNonNull(health, "health == null"); + return opExpiryEpochSeconds == 0 ? CONSTANTS.get(op).get(health) : new NodeStatus(health, op, opExpiryEpochSeconds); } @@ -240,19 +242,8 @@ public int hashCode() { @Override public String toString() { - return "OperationalState: " + operationalState - + "(expiry: " + opStateExpiryEpochSeconds + "s), Health: " + health; - } - - @Override - public int compareTo(NodeStatus o) { - int order = Boolean.compare(o.isHealthy(), isHealthy()); - if (order == 0) { - order = Boolean.compare(isDead(), o.isDead()); - } - if (order == 0) { - order = operationalState.compareTo(o.operationalState); - } - return order; + final String expiry = opStateExpiryEpochSeconds == 0 ? "no expiry" + : "expiry: " + opStateExpiryEpochSeconds + "s"; + return operationalState + "(" + expiry + ")-" + health; } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeAlreadyExistsException.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeAlreadyExistsException.java index 4341d74ec9dd..e99032680e68 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeAlreadyExistsException.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeAlreadyExistsException.java @@ -17,6 +17,8 @@ package org.apache.hadoop.hdds.scm.node.states; +import org.apache.hadoop.hdds.protocol.DatanodeID; + /** * This exception represents that there is already a node added to NodeStateMap * with same UUID. @@ -32,14 +34,9 @@ public NodeAlreadyExistsException() { } /** - * Constructs an {@code NodeAlreadyExistsException} with the specified - * detail message. - * - * @param message - * The detail message (which is saved for later retrieval - * by the {@link #getMessage()} method) + * Constructs an {@code NodeAlreadyExistsException} with the given {@link DatanodeID}. */ - public NodeAlreadyExistsException(String message) { - super(message); + public NodeAlreadyExistsException(DatanodeID datanodeID) { + super("Datanode " + datanodeID + " already exists"); } } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeStateMap.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeStateMap.java index 79c4346bf8d7..ab3f8fdea26f 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeStateMap.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/states/NodeStateMap.java @@ -80,7 +80,7 @@ public void addNode(DatanodeDetails datanodeDetails, NodeStatus nodeStatus, try { final DatanodeID id = datanodeDetails.getID(); if (nodeMap.containsKey(id)) { - throw new NodeAlreadyExistsException("Node UUID: " + id); + throw new NodeAlreadyExistsException(id); } nodeMap.put(id, new DatanodeInfo(datanodeDetails, nodeStatus, layoutInfo)); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/ECPipelineProvider.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/ECPipelineProvider.java index 1f884404c16d..b28a435de058 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/ECPipelineProvider.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/ECPipelineProvider.java @@ -97,6 +97,16 @@ protected Pipeline create(ECReplicationConfig replicationConfig, return createPipelineInternal(replicationConfig, nodes, dnIndexes); } + static final Comparator CREATE_FOR_READ_COMPARATOR = (left, right) -> { + final int healthy = Boolean.compare(right.isHealthy(), left.isHealthy()); + if (healthy != 0) { + return healthy; + } + final int dead = Boolean.compare(left.isDead(), right.isDead()); + return dead != 0 ? dead : left.getOperationalState().compareTo(right.getOperationalState()); + }; + + @Override public Pipeline createForRead( ECReplicationConfig replicationConfig, @@ -115,11 +125,11 @@ public Pipeline createForRead( nodeStatusMap.put(dn, nodeStatus); } } catch (NodeNotFoundException e) { - LOG.error("Node not found", e); + LOG.error("Failed to getNodeStatus for {}", dn, e); } } - dns.sort(Comparator.comparing(nodeStatusMap::get)); + dns.sort(Comparator.comparing(nodeStatusMap::get, CREATE_FOR_READ_COMPARATOR)); return createPipelineInternal(replicationConfig, dns, map); } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeStatus.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestCreateForReadComparator.java similarity index 66% rename from hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeStatus.java rename to hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestCreateForReadComparator.java index 01ceab3ab33c..f7b561a45b1e 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeStatus.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestCreateForReadComparator.java @@ -15,7 +15,7 @@ * limitations under the License. */ -package org.apache.hadoop.hdds.scm.node; +package org.apache.hadoop.hdds.scm.pipeline; import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.DECOMMISSIONING; import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.NodeOperationalState.ENTERING_MAINTENANCE; @@ -27,40 +27,52 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import java.util.Comparator; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.scm.node.NodeStatus; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; /** - * Tests for {@link NodeStatus}. + * Tests for {@link ECPipelineProvider#CREATE_FOR_READ_COMPARATOR}. */ -class TestNodeStatus { +class TestCreateForReadComparator { + private final Comparator comparator = ECPipelineProvider.CREATE_FOR_READ_COMPARATOR; + + int compare(NodeStatus left, NodeStatus right) { + return comparator.compare(left, right); + } @ParameterizedTest @EnumSource void readOnly(HddsProtos.NodeOperationalState state) { - assertEquals(0, NodeStatus.valueOf(state, HEALTHY) - .compareTo(NodeStatus.valueOf(state, HEALTHY_READONLY))); + assertEquals(0, compare( + NodeStatus.valueOf(state, HEALTHY), + NodeStatus.valueOf(state, HEALTHY_READONLY))); } @Test void healthyFirst() { - assertThat(0).isGreaterThan(inServiceHealthy().compareTo(inServiceStale())); - assertThat(0).isLessThan(inServiceDead().compareTo(inServiceHealthy())); - assertThat(0).isGreaterThan(NodeStatus.valueOf(ENTERING_MAINTENANCE, HEALTHY).compareTo( + assertThat(0).isGreaterThan(compare(inServiceHealthy(), inServiceStale())); + assertThat(0).isLessThan(compare(inServiceDead(), inServiceHealthy())); + assertThat(0).isGreaterThan(compare( + NodeStatus.valueOf(ENTERING_MAINTENANCE, HEALTHY), inServiceStale() )); - assertThat(0).isLessThan(inServiceStale().compareTo( + assertThat(0).isLessThan(compare( + inServiceStale(), NodeStatus.valueOf(DECOMMISSIONING, HEALTHY) )); } @Test void inServiceFirst() { - assertThat(0).isGreaterThan(inServiceHealthy().compareTo( + assertThat(0).isGreaterThan(compare( + inServiceHealthy(), NodeStatus.valueOf(ENTERING_MAINTENANCE, HEALTHY))); - assertThat(0).isLessThan(NodeStatus.valueOf(DECOMMISSIONING, HEALTHY).compareTo( + assertThat(0).isLessThan(compare( + NodeStatus.valueOf(DECOMMISSIONING, HEALTHY), inServiceHealthy() )); }