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 88ad6d1ec1 HDDS-8867. Datanode re-registration memory leak (#4916)
88ad6d1ec1 is described below
commit 88ad6d1ec19816c7123f1d70afe2fa1c3fdf80a6
Author: Doroszlai, Attila <[email protected]>
AuthorDate: Sat Jun 17 15:21:14 2023 +0200
HDDS-8867. Datanode re-registration memory leak (#4916)
---
.../hadoop/hdds/scm/node/SCMNodeManager.java | 48 ++++++++-----------
.../hadoop/hdds/scm/node/TestSCMNodeManager.java | 56 +++++++++-------------
2 files changed, 41 insertions(+), 63 deletions(-)
diff --git
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
index 721e6ef680..21b639ca8a 100644
---
a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
+++
b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java
@@ -20,7 +20,6 @@ package org.apache.hadoop.hdds.scm.node;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.apache.hadoop.hdds.DFSConfigKeysLegacy;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.DatanodeDetails;
@@ -401,7 +400,7 @@ public class SCMNodeManager implements NodeManager {
// Check that datanode in nodeStateManager has topology parent set
DatanodeDetails dn = nodeStateManager.getNode(datanodeDetails);
Preconditions.checkState(dn.getParent() != null);
- addEntryToDnsToUuidMap(dnsName, datanodeDetails.getUuidString());
+ addToDnsToUuidMap(dnsName, datanodeDetails.getUuidString());
// Updating Node Report, as registration is successful
processNodeReport(datanodeDetails, nodeReport);
LOG.info("Registered Data node : {}", datanodeDetails.toDebugString());
@@ -435,7 +434,7 @@ public class SCMNodeManager implements NodeManager {
} else {
oldDnsName = datanodeInfo.getIpAddress();
}
- updateEntryFromDnsToUuidMap(oldDnsName,
+ updateDnsToUuidMap(oldDnsName,
dnsName,
datanodeDetails.getUuidString());
@@ -464,38 +463,29 @@ public class SCMNodeManager implements NodeManager {
* running on that host. As each address can have many DNs running on it,
* this is a one to many mapping.
*
- * @param dnsName String representing the hostname or IP of the node
- * @param uuid String representing the UUID of the registered node.
+ * @param addr the hostname or IP of the node
+ * @param uuid the UUID of the registered node.
*/
- @SuppressFBWarnings(value =
"AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION")
- private synchronized void addEntryToDnsToUuidMap(
- String dnsName, String uuid) {
- Set<String> dnList = dnsToUuidMap.get(dnsName);
- if (dnList == null) {
- dnList = ConcurrentHashMap.newKeySet();
- dnsToUuidMap.put(dnsName, dnList);
- }
- dnList.add(uuid);
+ private synchronized void addToDnsToUuidMap(String addr, String uuid) {
+ dnsToUuidMap.computeIfAbsent(addr, k -> ConcurrentHashMap.newKeySet())
+ .add(uuid);
}
- private synchronized void removeEntryFromDnsToUuidMap(String dnsName) {
- if (!dnsToUuidMap.containsKey(dnsName)) {
- return;
- }
- Set<String> dnSet = dnsToUuidMap.get(dnsName);
- if (dnSet.contains(dnsName)) {
- dnSet.remove(dnsName);
- }
- if (dnSet.isEmpty()) {
- dnsToUuidMap.remove(dnsName);
+ private synchronized void removeFromDnsToUuidMap(String addr, String uuid) {
+ Set<String> dnSet = dnsToUuidMap.get(addr);
+ if (dnSet != null && dnSet.remove(uuid) && dnSet.isEmpty()) {
+ dnsToUuidMap.remove(addr);
}
}
- private synchronized void updateEntryFromDnsToUuidMap(String oldDnsName,
- String newDnsName,
- String uuid) {
- removeEntryFromDnsToUuidMap(oldDnsName);
- addEntryToDnsToUuidMap(newDnsName, uuid);
+ private synchronized void updateDnsToUuidMap(
+ String oldDnsName, String newDnsName, String uuid) {
+ Preconditions.checkNotNull(oldDnsName, "old address == null");
+ Preconditions.checkNotNull(newDnsName, "new address == null");
+ if (!oldDnsName.equals(newDnsName)) {
+ removeFromDnsToUuidMap(oldDnsName, uuid);
+ addToDnsToUuidMap(newDnsName, uuid);
+ }
}
/**
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
index 6a789cb92a..2f677629a0 100644
---
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
+++
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java
@@ -90,6 +90,7 @@ import java.util.Map;
import java.util.function.Predicate;
import java.util.stream.Collectors;
+import static java.util.Collections.emptyList;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
import static
org.apache.hadoop.fs.CommonConfigurationKeysPublic.NET_TOPOLOGY_NODE_SWITCH_MAPPING_IMPL_KEY;
@@ -121,6 +122,8 @@ import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import org.slf4j.Logger;
@@ -977,7 +980,7 @@ public class TestSCMNodeManager {
}
for (int i = 0; i < 5; i++) {
nodeManager.addDatanodeCommand(node1.getUuid(),
- new DeleteBlocksCommand(Collections.emptyList()));
+ new DeleteBlocksCommand(emptyList()));
}
Assertions.assertEquals(3, nodeManager.getTotalDatanodeCommandCount(
@@ -1063,7 +1066,7 @@ public class TestSCMNodeManager {
SCMCommand<?> createPipelineCommand =
new CreatePipelineCommand(PipelineID.randomId(),
HddsProtos.ReplicationType.RATIS,
- HddsProtos.ReplicationFactor.THREE, Collections.emptyList());
+ HddsProtos.ReplicationFactor.THREE, emptyList());
nodeManager.onMessage(
new CommandForDatanode<>(datanode1, closeContainerCommand), null);
@@ -1525,7 +1528,7 @@ public class TestSCMNodeManager {
StorageReportProto report = HddsTestUtils
.createStorageReport(dnId, storagePath, capacity, used, free,
null);
nodeManager.register(dn, HddsTestUtils.createNodeReport(
- Arrays.asList(report), Collections.emptyList()), null);
+ Arrays.asList(report), emptyList()), null);
nodeManager.processHeartbeat(dn, layoutInfo);
}
//TODO: wait for EventQueue to be processed
@@ -1578,7 +1581,7 @@ public class TestSCMNodeManager {
failed = !failed;
}
nodeManager.register(dn, HddsTestUtils.createNodeReport(reports,
- Collections.emptyList()), null);
+ emptyList()), null);
LayoutVersionManager versionManager =
nodeManager.getLayoutVersionManager();
LayoutVersionProto layoutInfo = toLayoutVersionProto(
@@ -1635,7 +1638,7 @@ public class TestSCMNodeManager {
.createStorageReport(dnId, storagePath, capacity, scmUsed,
remaining, null);
NodeReportProto nodeReportProto = HddsTestUtils.createNodeReport(
- Arrays.asList(report), Collections.emptyList());
+ Arrays.asList(report), emptyList());
nodeReportHandler.onMessage(
new NodeReportFromDatanode(datanodeDetails, nodeReportProto),
publisher);
@@ -1767,7 +1770,7 @@ public class TestSCMNodeManager {
nodemanager
.register(datanodeDetails, HddsTestUtils.createNodeReport(
- Arrays.asList(report), Collections.emptyList()),
+ Arrays.asList(report), emptyList()),
HddsTestUtils.getRandomPipelineReports());
eq.fireEvent(DATANODE_COMMAND,
new CommandForDatanode<>(datanodeDetails.getUuid(),
@@ -1813,25 +1816,6 @@ public class TestSCMNodeManager {
testScmRegisterNodeWithNetworkTopology(true);
}
- /**
- * Test getNodesByAddress when using IPs.
- *
- */
- @Test
- public void testgetNodesByAddressWithIpAddress()
- throws IOException, InterruptedException, AuthenticationException {
- testGetNodesByAddress(false);
- }
-
- /**
- * Test getNodesByAddress when using hostnames.
- */
- @Test
- public void testgetNodesByAddressWithHostname()
- throws IOException, InterruptedException, AuthenticationException {
- testGetNodesByAddress(true);
- }
-
/**
* Test add node into a 4-layer network topology during node register.
*/
@@ -1948,7 +1932,7 @@ public class TestSCMNodeManager {
remaining, null);
nodeManager.register(datanodeDetails, HddsTestUtils.createNodeReport(
- Arrays.asList(report), Collections.emptyList()),
+ Arrays.asList(report), emptyList()),
HddsTestUtils.getRandomPipelineReports());
LayoutVersionManager versionManager =
@@ -1958,7 +1942,7 @@ public class TestSCMNodeManager {
versionManager.getSoftwareLayoutVersion());
nodeManager.register(datanodeDetails,
HddsTestUtils.createNodeReport(Arrays.asList(report),
- Collections.emptyList()),
+ emptyList()),
HddsTestUtils.getRandomPipelineReports(), layoutInfo);
nodeManager.processHeartbeat(datanodeDetails, layoutInfo);
if (i == 5) {
@@ -1992,20 +1976,21 @@ public class TestSCMNodeManager {
/**
* Test add node into a 4-layer network topology during node register.
*/
- private void testGetNodesByAddress(boolean useHostname)
- throws IOException, InterruptedException, AuthenticationException {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ void testGetNodesByAddress(boolean useHostname)
+ throws IOException, AuthenticationException {
OzoneConfiguration conf = getConf();
conf.setTimeDuration(OZONE_SCM_HEARTBEAT_PROCESS_INTERVAL, 1000,
MILLISECONDS);
+ conf.setBoolean(DFSConfigKeysLegacy.DFS_DATANODE_USE_DN_HOSTNAME,
+ useHostname);
// create a set of hosts - note two hosts on "host1"
String[] hostNames = {"host1", "host1", "host2", "host3", "host4"};
String[] ipAddress =
{"1.2.3.4", "1.2.3.4", "2.3.4.5", "3.4.5.6", "4.5.6.7"};
- if (useHostname) {
- conf.set(DFSConfigKeysLegacy.DFS_DATANODE_USE_DN_HOSTNAME, "true");
- }
final int nodeCount = hostNames.length;
try (SCMNodeManager nodeManager = createNodeManager(conf)) {
for (int i = 0; i < nodeCount; i++) {
@@ -2070,7 +2055,7 @@ public class TestSCMNodeManager {
assertEquals(hostName, returnedNode.getHostName());
assertTrue(returnedNode.getNetworkLocation()
.startsWith("/rack1/ng"));
- assertTrue(returnedNode.getParent() != null);
+ assertNotNull(returnedNode.getParent());
// test updating ip address and host name
String updatedIpAddress = "2.3.4.5";
@@ -2091,7 +2076,10 @@ public class TestSCMNodeManager {
assertEquals(updatedHostName, returnedUpdatedNode.getHostName());
assertTrue(returnedUpdatedNode.getNetworkLocation()
.startsWith("/rack1/ng"));
- assertTrue(returnedUpdatedNode.getParent() != null);
+ assertNotNull(returnedUpdatedNode.getParent());
+
+ assertEquals(emptyList(), nodeManager.getNodesByAddress(hostName));
+ assertEquals(emptyList(), nodeManager.getNodesByAddress(ipAddress));
}
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
