sunchao commented on a change in pull request #2080:
URL: https://github.com/apache/hadoop/pull/2080#discussion_r442384275
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
##########
@@ -502,12 +479,27 @@ private DatanodeInfo chooseDatanode(final Router router,
final LocatedBlocks locations = cp.getBlockLocations(path, offset, 1);
final int count = locations.locatedBlockCount();
if (count > 0) {
+ if (excludeDatanodes != null) {
+ Collection<String> collection =
+ getTrimmedStringCollection(excludeDatanodes);
+ dns = getDatanodeReport(router);
+ for (DatanodeInfo dn : dns) {
+ if (collection.contains(dn.getName())) {
+ excludes.add(dn);
+ }
+ }
+ }
+
LocatedBlock location0 = locations.get(0);
return bestNode(location0.getLocations(), excludes);
}
}
}
+ if (dns == null) {
Review comment:
nit: this is always true.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
##########
@@ -502,12 +479,27 @@ private DatanodeInfo chooseDatanode(final Router router,
final LocatedBlocks locations = cp.getBlockLocations(path, offset, 1);
final int count = locations.locatedBlockCount();
if (count > 0) {
+ if (excludeDatanodes != null) {
+ Collection<String> collection =
+ getTrimmedStringCollection(excludeDatanodes);
+ dns = getDatanodeReport(router);
+ for (DatanodeInfo dn : dns) {
+ if (collection.contains(dn.getName())) {
+ excludes.add(dn);
+ }
+ }
+ }
+
LocatedBlock location0 = locations.get(0);
return bestNode(location0.getLocations(), excludes);
}
}
}
+ if (dns == null) {
+ dns = getDatanodeReport(router);
+ }
+
return getRandomDatanode(dns, excludes);
Review comment:
In this case, `excludes` is always empty. We need to compute it using
the `dns` obtained.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
##########
@@ -564,4 +556,28 @@ public Credentials createCredentials(
renewer != null? renewer: ugi.getShortUserName());
return c;
}
+
+ /**
+ * Get the datanode report from all namespaces that are registered
+ * and active in the federation.
+ * @param router Router from which to get the report.
Review comment:
nit: leave a blank line before the `@param` line.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
##########
@@ -564,4 +556,28 @@ public Credentials createCredentials(
renewer != null? renewer: ugi.getShortUserName());
return c;
}
+
+ /**
+ * Get the datanode report from all namespaces that are registered
+ * and active in the federation.
+ * @param router Router from which to get the report.
+ * @return List of datanodes.
+ * @throws IOException If it cannot get the RPC Server.
+ */
+ private static DatanodeInfo[] getDatanodeReport(
+ final Router router) throws IOException {
+ // We need to get the DNs as a privileged user
+ final RouterRpcServer rpcServer = getRPCServer(router);
Review comment:
should we wrap these in the `try .. catch` block? and what is the return
value if `rpcServer.getDatanodeReport` fail? null?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
