This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 918bb9809c HDDS-12626. Move the compare method in NodeStatus to
ECPipelineProvider. (#8116)
918bb9809c is described below
commit 918bb9809c8731ff04645e4634a0955b01dc115a
Author: Tsz-Wo Nicholas Sze <[email protected]>
AuthorDate: Thu Mar 20 07:58:16 2025 -0700
HDDS-12626. Move the compare method in NodeStatus to ECPipelineProvider.
(#8116)
---
.../apache/hadoop/hdds/scm/node/NodeStatus.java | 31 +++++++-------------
.../node/states/NodeAlreadyExistsException.java | 13 ++++-----
.../hadoop/hdds/scm/node/states/NodeStateMap.java | 2 +-
.../hdds/scm/pipeline/ECPipelineProvider.java | 14 +++++++--
.../TestCreateForReadComparator.java} | 34 +++++++++++++++-------
5 files changed, 52 insertions(+), 42 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 9b9be03a49..a67525acce 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 @@
* <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()) {
@@ -53,19 +53,21 @@ public final class NodeStatus implements
Comparable<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 = 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);
}
@@ -250,19 +252,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 4341d74ec9..e99032680e 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 ce49cb6c64..0f4420f8a6 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 1f884404c1..b28a435de0 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<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);
}
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 01ceab3ab3..f7b561a45b 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<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()
));
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
