HDDS-12626. Move the compare method in NodeStatus to ECPipelineProvider.

hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeStatus.java

@@ -43,29 +43,31 @@
  * <p>
  * This class is value-based.
  */
-public final class NodeStatus implements Comparable<NodeStatus> {
+public final class NodeStatus {
   /** For the {@link NodeStatus} objects with {@link #opStateExpiryEpochSeconds} == 0. */
-  private static final Map<NodeOperationalState, Map<NodeState, NodeStatus>> CONSTANT_MAP;
+  private static final Map<NodeOperationalState, Map<NodeState, NodeStatus>> CONSTANTS;
   static {
     final Map<NodeOperationalState, Map<NodeState, NodeStatus>> map = new EnumMap<>(NodeOperationalState.class);
     for (NodeOperationalState op : NodeOperationalState.values()) {
       final EnumMap<NodeState, NodeStatus> healthMap = new EnumMap<>(NodeState.class);
       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;
   }
 }

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");
   }
 }

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));

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<NodeStatus> 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);
   }

hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestNodeStatus.java → 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<NodeStatus> 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()
     ));
   }