From 1c1b465ea473541fc0b827f7afd8228240d05421 Mon Sep 17 00:00:00 2001 From: Andrey Yarovoy Date: Tue, 4 Aug 2026 12:20:42 -0400 Subject: [PATCH] HDDS-16043. Use EnumMap for ports in DatanodeDetails --- .../hadoop/hdds/protocol/DatanodeDetails.java | 69 +++++++++---------- .../hdds/protocol/TestDatanodeDetails.java | 37 ++++++++++ 2 files changed, 69 insertions(+), 37 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java index 4abe44a3040b..2ea4fa64213b 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java @@ -30,12 +30,13 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Collections; +import java.util.EnumMap; import java.util.EnumSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.UUID; -import java.util.stream.Collectors; import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.hdds.DatanodeVersion; import org.apache.hadoop.hdds.HddsUtils; @@ -84,7 +85,7 @@ public class DatanodeDetails extends NodeImpl implements Comparable ports; + private final Map ports; private String certSerialId; private String version; private long setupTime; @@ -244,10 +245,8 @@ public StringWithByteString getHostNameAsByteString() { * @param port DataNode port */ public synchronized void setPort(Port port) { - // If the port is already in the list remove it first and add the - // new/updated port value. - ports.remove(port); - ports.add(port); + // Overwrites any existing port with the same name. + ports.put(port.getName(), port); } public synchronized void setPort(Name name, int port) { @@ -272,11 +271,11 @@ public void setStandalonePort(int port) { * @return DataNode Ports */ public synchronized List getPorts() { - return new ArrayList<>(ports); + return new ArrayList<>(ports.values()); } public synchronized boolean hasPort(int port) { - for (Port p : ports) { + for (Port p : ports.values()) { if (p.getValue() == port) { return true; } @@ -356,51 +355,46 @@ public void setPersistedOpStateExpiryEpochSec(long expiry) { * @return Port */ public synchronized Port getPort(Port.Name name) { - Port ratisPort = null; - for (Port port : ports) { - if (port.getName().equals(name)) { - return port; - } - if (port.getName().equals(Name.RATIS)) { - ratisPort = port; - } + final Port port = ports.get(name); + if (port != null) { + return port; } // if no separate admin/server/datastream port, // return single Ratis one for compatibility if (name == Name.RATIS_ADMIN || name == Name.RATIS_SERVER || name == Name.RATIS_DATASTREAM) { - return ratisPort; + return ports.get(Name.RATIS); } return null; } - // CHANGE: add a helper to check whether a port is explicitly present - // without applying compatibility fallback. + // Checks whether a port is explicitly present without applying the + // compatibility fallback in getPort. public synchronized boolean hasPort(Port.Name name) { - for (Port port : ports) { - if (port.getName().equals(name)) { - return true; - } - } - return false; + return ports.containsKey(name); } /** * Whether this datanode's exposed ports differ from {@code other}'s. - * Compared as a set of name=value entries, since {@link Port#equals} - * ignores the port value. + * Compared by name and value, since {@link Port#equals} ignores the port + * value. * * @param other another snapshot of this datanode * @return true if the two port sets are not identical */ public boolean portsChanged(DatanodeDetails other) { - return !portValues(this).equals(portValues(other)); + return !portValues().equals(other.portValues()); } - private static Set portValues(DatanodeDetails datanodeDetails) { - return datanodeDetails.getPorts().stream() - .map(port -> port.getName() + "=" + port.getValue()) - .collect(Collectors.toSet()); + /** + * A name-to-value snapshot of this datanode's ports, taken under its lock + * so {@link #portsChanged} can compare two nodes value-aware without holding + * both locks at once. + */ + private synchronized Map portValues() { + final Map values = new EnumMap<>(Port.Name.class); + ports.forEach((name, port) -> values.put(name, port.getValue())); + return values; } /** @@ -595,7 +589,7 @@ public HddsProtos.DatanodeDetailsProto.Builder toProtoBuilder( .compareTo(VERSION_HANDLES_UNKNOWN_DN_PORTS) >= 0; final int requestedPortCount = filterPorts.size(); final boolean maySkip = requestedPortCount > 0; - for (Port port : ports) { + for (Port port : ports.values()) { if (maySkip && !filterPorts.contains(port.getName())) { if (LOG.isDebugEnabled()) { LOG.debug("Skip adding {} port {} to proto message", @@ -738,7 +732,7 @@ public static final class Builder { private StringWithByteString networkName; private StringWithByteString networkLocation; private int level; - private List ports; + private Map ports; private String certSerialId; private String version; private long setupTime; @@ -753,7 +747,7 @@ public static final class Builder { * DatanodeDetails#newBuilder. */ private Builder() { - ports = new ArrayList<>(); + ports = new EnumMap<>(Port.Name.class); } /** @@ -769,7 +763,8 @@ public Builder setDatanodeDetails(DatanodeDetails details) { this.networkName = details.getNetworkNameAsByteString(); this.networkLocation = details.getNetworkLocationAsByteString(); this.level = details.getLevel(); - this.ports = details.getPorts(); + this.ports = new EnumMap<>(Port.Name.class); + details.getPorts().forEach(this::addPort); this.certSerialId = details.getCertSerialId(); this.version = details.getVersion(); this.setupTime = details.getSetupTime(); @@ -885,7 +880,7 @@ public Builder setLevel(int level) { * @return DatanodeDetails.Builder */ public Builder addPort(Port port) { - this.ports.add(port); + this.ports.put(port.getName(), port); return this; } diff --git a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/protocol/TestDatanodeDetails.java b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/protocol/TestDatanodeDetails.java index dcbf9553dd03..8994044e4b62 100644 --- a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/protocol/TestDatanodeDetails.java +++ b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/protocol/TestDatanodeDetails.java @@ -23,6 +23,8 @@ import static org.apache.hadoop.ozone.ClientVersion.VERSION_HANDLES_UNKNOWN_DN_PORTS; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableSet; import java.util.Set; @@ -86,6 +88,41 @@ public void testNewBuilderCurrentVersion() { assertEquals(DatanodeVersion.CURRENT.toProtoValue(), dn3.getCurrentVersion()); } + @Test + void portsChangedComparesNameAndValue() { + DatanodeID id = DatanodeID.randomID(); + DatanodeDetails base = DatanodeDetails.newBuilder() + .setID(id) + .addPort(DatanodeDetails.newStandalonePort(9858)) + .addPort(DatanodeDetails.newRatisPort(9859)) + .build(); + + // Identical name/value set: no change. + DatanodeDetails same = DatanodeDetails.newBuilder() + .setID(id) + .addPort(DatanodeDetails.newStandalonePort(9858)) + .addPort(DatanodeDetails.newRatisPort(9859)) + .build(); + assertFalse(base.portsChanged(same)); + + // Same names, one different value: detected (Port.equals ignores value). + DatanodeDetails changedValue = DatanodeDetails.newBuilder() + .setID(id) + .addPort(DatanodeDetails.newStandalonePort(9858)) + .addPort(DatanodeDetails.newRatisPort(1234)) + .build(); + assertTrue(base.portsChanged(changedValue)); + + // Extra port: detected (key set differs). + DatanodeDetails extraPort = DatanodeDetails.newBuilder() + .setID(id) + .addPort(DatanodeDetails.newStandalonePort(9858)) + .addPort(DatanodeDetails.newRatisPort(9859)) + .addPort(DatanodeDetails.newPort(Name.RATIS_DATASTREAM, 9860)) + .build(); + assertTrue(base.portsChanged(extraPort)); + } + public static void assertPorts(HddsProtos.DatanodeDetailsProto dn, Set expectedPorts) throws IllegalArgumentException { assertEquals(expectedPorts.size(), dn.getPortsCount());