ashishkumar50 commented on code in PR #5458:
URL: https://github.com/apache/ozone/pull/5458#discussion_r1363236888
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -139,26 +142,77 @@ private List<DatanodeDetails>
mapHostnamesToDatanodes(List<String> hosts)
+ " Please check the port number.");
}
results.add(found.get(0));
- } else if (found.size() > 1) {
- DatanodeDetails match = null;
- for (DatanodeDetails dn : found) {
- if (validateDNPortMatch(host.getPort(), dn)) {
- match = dn;
- break;
+ } else {
+ // Here we either have multiple DNs on the same host / IP, and they
+ // should have different ports. Or, we have a case where a DN was
+ // registered from a host, then stopped and formatted, changing its
+ // UUID and registered again. In that case, the ports of all hosts
+ // should be the same, and we should just use the one with the most
+ // recent heartbeat.
+ if (allPortsMatch(found)) {
+ // All ports match, so just use the most recent heartbeat as it is
+ // not possible for a host to have 2 DNs coming from the same port.
+ DatanodeDetails mostRecent = findDnWithMostRecentHeartbeat(found);
+ if (mostRecent == null) {
+ throw new InvalidHostStringException("Host " +
host.getRawHostname()
+ + " has multiple datanodes registered with SCM."
+ + " All have identical ports, but none have a newest"
+ + " heartbeat.");
}
+ results.add(mostRecent);
+ } else {
+ DatanodeDetails match = null;
+ for (DatanodeDetails dn : found) {
Review Comment:
Here can we have the below possibility?
Let's say it matches 3 DNs in which 2 DN are having same port but not the
third one.
And assume` host port `matches the 2 DN.
In this case it will not able to choose which one to decommission and here
also we need `findDnWithMostRecentHeartbeat` among the 2 DN
Can you consider `host port` also in `allPortsMatch` so that this condition
will not arise?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -139,26 +142,77 @@ private List<DatanodeDetails>
mapHostnamesToDatanodes(List<String> hosts)
+ " Please check the port number.");
}
results.add(found.get(0));
- } else if (found.size() > 1) {
- DatanodeDetails match = null;
- for (DatanodeDetails dn : found) {
- if (validateDNPortMatch(host.getPort(), dn)) {
- match = dn;
- break;
+ } else {
+ // Here we either have multiple DNs on the same host / IP, and they
+ // should have different ports. Or, we have a case where a DN was
+ // registered from a host, then stopped and formatted, changing its
+ // UUID and registered again. In that case, the ports of all hosts
+ // should be the same, and we should just use the one with the most
+ // recent heartbeat.
+ if (allPortsMatch(found)) {
+ // All ports match, so just use the most recent heartbeat as it is
+ // not possible for a host to have 2 DNs coming from the same port.
+ DatanodeDetails mostRecent = findDnWithMostRecentHeartbeat(found);
+ if (mostRecent == null) {
+ throw new InvalidHostStringException("Host " +
host.getRawHostname()
+ + " has multiple datanodes registered with SCM."
+ + " All have identical ports, but none have a newest"
+ + " heartbeat.");
}
+ results.add(mostRecent);
+ } else {
+ DatanodeDetails match = null;
+ for (DatanodeDetails dn : found) {
+ if (validateDNPortMatch(host.getPort(), dn)) {
+ match = dn;
+ break;
+ }
+ }
+ if (match == null) {
+ throw new InvalidHostStringException("Host " +
host.getRawHostname()
+ + " is running multiple datanodes registered with SCM,"
+ + " but no port numbers match."
+ + " Please check the port number.");
+ }
+ results.add(match);
}
- if (match == null) {
- throw new InvalidHostStringException("Host " + host.getRawHostname()
- + " is running multiple datanodes registered with SCM,"
- + " but no port numbers match."
- + " Please check the port number.");
- }
- results.add(match);
}
}
return results;
}
+ public boolean allPortsMatch(List<DatanodeDetails> dns) {
+ if (dns.size() < 2) {
+ return true;
Review Comment:
Currently this looks like it will not hit for dns with size < 2. We are
returning `true` which can call `findDnWithMostRecentHeartbeat` and we are
trying to access two dn heartbeats which will fail.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/NodeDecommissionManager.java:
##########
@@ -139,26 +142,77 @@ private List<DatanodeDetails>
mapHostnamesToDatanodes(List<String> hosts)
+ " Please check the port number.");
}
results.add(found.get(0));
- } else if (found.size() > 1) {
- DatanodeDetails match = null;
- for (DatanodeDetails dn : found) {
- if (validateDNPortMatch(host.getPort(), dn)) {
- match = dn;
- break;
+ } else {
+ // Here we either have multiple DNs on the same host / IP, and they
+ // should have different ports. Or, we have a case where a DN was
+ // registered from a host, then stopped and formatted, changing its
+ // UUID and registered again. In that case, the ports of all hosts
+ // should be the same, and we should just use the one with the most
+ // recent heartbeat.
+ if (allPortsMatch(found)) {
+ // All ports match, so just use the most recent heartbeat as it is
+ // not possible for a host to have 2 DNs coming from the same port.
+ DatanodeDetails mostRecent = findDnWithMostRecentHeartbeat(found);
+ if (mostRecent == null) {
+ throw new InvalidHostStringException("Host " +
host.getRawHostname()
+ + " has multiple datanodes registered with SCM."
+ + " All have identical ports, but none have a newest"
+ + " heartbeat.");
}
+ results.add(mostRecent);
+ } else {
+ DatanodeDetails match = null;
+ for (DatanodeDetails dn : found) {
+ if (validateDNPortMatch(host.getPort(), dn)) {
+ match = dn;
+ break;
+ }
+ }
+ if (match == null) {
+ throw new InvalidHostStringException("Host " +
host.getRawHostname()
+ + " is running multiple datanodes registered with SCM,"
+ + " but no port numbers match."
+ + " Please check the port number.");
+ }
+ results.add(match);
}
- if (match == null) {
- throw new InvalidHostStringException("Host " + host.getRawHostname()
- + " is running multiple datanodes registered with SCM,"
- + " but no port numbers match."
- + " Please check the port number.");
- }
- results.add(match);
}
}
return results;
}
+ public boolean allPortsMatch(List<DatanodeDetails> dns) {
Review Comment:
Both methods we can change to private.
--
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]
