From 544429d563021bd78898fa88e05d86a222f881a6 Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Tue, 18 Mar 2025 11:33:34 -0700 Subject: [PATCH 1/5] HDDS-12626. Move the compare method in NodeStatus to ECPipelineProvider. --- .../hadoop/hdds/scm/node/NodeStatus.java | 16 ++------- .../states/NodeAlreadyExistsException.java | 10 +++--- .../hdds/scm/node/states/NodeStateMap.java | 2 +- .../hdds/scm/pipeline/ECPipelineProvider.java | 14 ++++++-- .../TestCreateForReadComparator.java} | 34 +++++++++++++------ 5 files changed, 42 insertions(+), 34 deletions(-) rename hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/{node/TestNodeStatus.java => pipeline/TestCreateForReadComparator.java} (66%) 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..350379574994 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,7 +43,7 @@ *

* 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; static { @@ -55,7 +55,7 @@ public final class NodeStatus implements Comparable { } map.put(op, healthMap); } - CONSTANT_MAP = map; + CONSTANT_MAP = Collections.unmodifiableMap(map); } /** @return a {@link NodeStatus} object with {@link #opStateExpiryEpochSeconds} == 0. */ @@ -243,16 +243,4 @@ 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; - } } 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..3c0eb97bf238 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. @@ -34,12 +36,8 @@ 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) */ - 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() )); } From 4cab15bdd0d19207234701324ca55ec262c9a8ff Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Wed, 19 Mar 2025 13:02:57 -0700 Subject: [PATCH 2/5] unmodifiableMap --- .../main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 350379574994..61edf7fbe090 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 @@ -53,7 +53,7 @@ public final class NodeStatus { 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 = Collections.unmodifiableMap(map); } From 0612de8a0585013a8182c263d14ad05a129e2bcc Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Wed, 19 Mar 2025 16:48:23 -0700 Subject: [PATCH 3/5] requireNonNull --- .../org/apache/hadoop/hdds/scm/node/NodeStatus.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 61edf7fbe090..328681dbd99a 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 @@ -45,7 +45,7 @@ */ 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()) { @@ -55,16 +55,20 @@ public final class NodeStatus { } map.put(op, Collections.unmodifiableMap(healthMap)); } - CONSTANT_MAP = Collections.unmodifiableMap(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); + Objects.requireNonNull(op, "op == null"); + Objects.requireNonNull(health, "health == null"); + return CONSTANTS.get(op).get(health); } /** @return a {@link NodeStatus} object. */ public static NodeStatus valueOf(NodeOperationalState op, NodeState health, long opExpiryEpochSeconds) { + Objects.requireNonNull(op, "op == null"); + Objects.requireNonNull(health, "health == null"); return opExpiryEpochSeconds == 0 ? valueOf(op, health) : new NodeStatus(health, op, opExpiryEpochSeconds); } From b6c8b6f4fe94450e9eff40eca7ab4d808d21c7f5 Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Wed, 19 Mar 2025 16:52:17 -0700 Subject: [PATCH 4/5] toString --- .../java/org/apache/hadoop/hdds/scm/node/NodeStatus.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 328681dbd99a..909bd3b807e4 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 @@ -244,7 +244,8 @@ public int hashCode() { @Override public String toString() { - return "OperationalState: " + operationalState - + "(expiry: " + opStateExpiryEpochSeconds + "s), Health: " + health; + final String expiry = opStateExpiryEpochSeconds == 0 ? "no expiry" + : "expiry: " + opStateExpiryEpochSeconds + "s"; + return operationalState + "(" + expiry + ")-" + health; } } From 5ee52d3123bc53dd1766a4db880e82b1e1295c0e Mon Sep 17 00:00:00 2001 From: Tsz-Wo Nicholas Sze Date: Thu, 20 Mar 2025 05:50:13 -0700 Subject: [PATCH 5/5] Address review comments. --- .../java/org/apache/hadoop/hdds/scm/node/NodeStatus.java | 6 ++---- .../hdds/scm/node/states/NodeAlreadyExistsException.java | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) 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 909bd3b807e4..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 @@ -60,16 +60,14 @@ public final class NodeStatus { /** @return a {@link NodeStatus} object with {@link #opStateExpiryEpochSeconds} == 0. */ public static NodeStatus valueOf(NodeOperationalState op, NodeState health) { - Objects.requireNonNull(op, "op == null"); - Objects.requireNonNull(health, "health == null"); - return CONSTANTS.get(op).get(health); + return valueOf(op, health, 0); } /** @return a {@link NodeStatus} object. */ public static NodeStatus valueOf(NodeOperationalState op, NodeState health, long opExpiryEpochSeconds) { Objects.requireNonNull(op, "op == null"); Objects.requireNonNull(health, "health == null"); - return opExpiryEpochSeconds == 0 ? valueOf(op, health) + return opExpiryEpochSeconds == 0 ? CONSTANTS.get(op).get(health) : new NodeStatus(health, op, opExpiryEpochSeconds); } 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 3c0eb97bf238..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 @@ -34,8 +34,7 @@ public NodeAlreadyExistsException() { } /** - * Constructs an {@code NodeAlreadyExistsException} with the specified - * detail message. + * Constructs an {@code NodeAlreadyExistsException} with the given {@link DatanodeID}. */ public NodeAlreadyExistsException(DatanodeID datanodeID) { super("Datanode " + datanodeID + " already exists");