Copilot commented on code in PR #8852:
URL: https://github.com/apache/hadoop/pull/8852#discussion_r4343100649
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestDatanodeManager.java:
##########
@@ -195,6 +197,42 @@ public void testHost2NodeMapCorrectAfterReregister()
assertNotNull(dm.getDatanodeByHostName(hostnameNew), "should be a node
with new hostname");
}
+ /**
+ * Slow peer reports keyed by IPv4, IPv6 and bracketed IPv6 info addresses
+ * must all resolve to their registered datanodes.
+ */
+ @Test
+ public void testSlowPeersResolvedForIpv4AndIpv6() throws IOException {
+ FSNamesystem fsn = Mockito.mock(FSNamesystem.class);
+ Mockito.when(fsn.hasWriteLock()).thenReturn(true);
+ Mockito.when(fsn.hasWriteLock(RwLockMode.BM)).thenReturn(true);
+ Configuration conf = new Configuration();
+ conf.setBoolean(DFSConfigKeys.DFS_DATANODE_PEER_STATS_ENABLED_KEY, true);
+ DatanodeManager dm = mockDatanodeManager(fsn, conf);
+ try {
+ String[] ips = {"1.2.3.4", "2001:db8::1", "2001:db8::2", "5.6.7.8"};
+ for (int i = 0; i < ips.length; i++) {
+ dm.registerDatanode(new DatanodeRegistration(
+ new DatanodeID(ips[i], "", "uuid" + i, 9866, 9864, 0, 9867), null,
null, "version"));
+ }
+ DatanodeDescriptor ipv4 = dm.getDatanodeByHost(ips[0]);
+ DatanodeDescriptor ipv6 = dm.getDatanodeByHost(ips[1]);
+ DatanodeDescriptor ipv6Bracketed = dm.getDatanodeByHost(ips[2]);
+
+ SlowPeerTracker tracker = dm.getSlowPeerTracker();
+ OutlierMetrics metrics = new OutlierMetrics(0.0, 0.0, 0.0, 5.0);
+ tracker.addReport(ipv4.getInfoAddr(), ips[3], metrics);
+ tracker.addReport(ipv6.getInfoAddr(), ips[3], metrics);
Review Comment:
This test intends to cover 'plain IPv6' and 'bracketed IPv6' slow-peer keys,
but `ipv6.getInfoAddr()` may already be bracketed depending on how the info
address is formatted in the implementation, which can make the 'plain IPv6'
case not actually exercised. To ensure the intended coverage, construct the
unbracketed key explicitly (e.g., `ips[1] + \":\" + ipv6.getInfoPort()`) and
keep the bracketed case via `NetUtils.getHostPortString(...)`.
##########
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestDatanodeManager.java:
##########
@@ -195,6 +197,42 @@ public void testHost2NodeMapCorrectAfterReregister()
assertNotNull(dm.getDatanodeByHostName(hostnameNew), "should be a node
with new hostname");
}
+ /**
+ * Slow peer reports keyed by IPv4, IPv6 and bracketed IPv6 info addresses
+ * must all resolve to their registered datanodes.
+ */
+ @Test
+ public void testSlowPeersResolvedForIpv4AndIpv6() throws IOException {
+ FSNamesystem fsn = Mockito.mock(FSNamesystem.class);
+ Mockito.when(fsn.hasWriteLock()).thenReturn(true);
+ Mockito.when(fsn.hasWriteLock(RwLockMode.BM)).thenReturn(true);
+ Configuration conf = new Configuration();
+ conf.setBoolean(DFSConfigKeys.DFS_DATANODE_PEER_STATS_ENABLED_KEY, true);
+ DatanodeManager dm = mockDatanodeManager(fsn, conf);
+ try {
+ String[] ips = {"1.2.3.4", "2001:db8::1", "2001:db8::2", "5.6.7.8"};
+ for (int i = 0; i < ips.length; i++) {
+ dm.registerDatanode(new DatanodeRegistration(
+ new DatanodeID(ips[i], "", "uuid" + i, 9866, 9864, 0, 9867), null,
null, "version"));
+ }
+ DatanodeDescriptor ipv4 = dm.getDatanodeByHost(ips[0]);
+ DatanodeDescriptor ipv6 = dm.getDatanodeByHost(ips[1]);
+ DatanodeDescriptor ipv6Bracketed = dm.getDatanodeByHost(ips[2]);
+
+ SlowPeerTracker tracker = dm.getSlowPeerTracker();
+ OutlierMetrics metrics = new OutlierMetrics(0.0, 0.0, 0.0, 5.0);
+ tracker.addReport(ipv4.getInfoAddr(), ips[3], metrics);
+ tracker.addReport(ipv6.getInfoAddr(), ips[3], metrics);
+ tracker.addReport(NetUtils.getHostPortString(ips[2],
ipv6Bracketed.getInfoPort()), ips[3],
+ metrics);
+
+ assertThat(dm.getAllSlowDataNodes()).containsExactlyInAnyOrder(ipv4,
ipv6, ipv6Bracketed);
Review Comment:
The variable name `ipv6Bracketed` is misleading: it holds a
`DatanodeDescriptor` looked up by an unbracketed IPv6 literal (`ips[2]`), and
the bracketed form is only used later when constructing the slow-peer key.
Renaming to something like `ipv6ForBracketedKey` (or `ipv6_2`) would make the
test intent clearer.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeManager.java:
##########
@@ -2264,7 +2264,11 @@ private List<DatanodeDescriptor>
getDnDescriptorsFromIpAddr(List<String> nodes)
if (StringUtils.isBlank(node) || !node.contains(IP_PORT_SEPARATOR)) {
continue;
}
- String ipAddr = node.split(IP_PORT_SEPARATOR)[0];
+ // Port is after the last ':'; IPv6 hosts may be bracketed.
+ String ipAddr = node.substring(0, node.lastIndexOf(IP_PORT_SEPARATOR));
+ if (ipAddr.startsWith("[") && ipAddr.endsWith("]")) {
+ ipAddr = ipAddr.substring(1, ipAddr.length() - 1);
+ }
Review Comment:
The current guard `!node.contains(\":\")` is too weak: a bare IPv6 address
(no port) contains ':' and will be mis-parsed by `substring(0,
lastIndexOf(':'))`, producing an incorrect host that can never match (and
potentially hiding malformed inputs). Consider tightening parsing to require an
actual host+port form: for bracketed IPv6 require a `\"]:\"` delimiter;
otherwise split on the last ':' only if the suffix is a valid numeric port (and
skip/continue if not). This prevents accidentally treating plain IPv6 literals
as host:port keys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
