szetszwo commented on code in PR #10633:
URL: https://github.com/apache/ozone/pull/10633#discussion_r3555200487
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java:
##########
@@ -2204,32 +2201,92 @@ private void sortDatanodes(String clientMachine,
List<OmKeyInfo> keyInfos) {
@VisibleForTesting
public List<? extends DatanodeDetails> sortDatanodes(List<? extends
DatanodeDetails> nodes,
String clientMachine) {
- final Node client = getClientNode(clientMachine, nodes);
- return ozoneManager.getClusterMap()
- .sortByDistanceCost(client, nodes, nodes.size());
+ final NetworkTopology clusterMap = ozoneManager.getClusterMap();
+ final Node client = getClientNode(clientMachine, nodes, clusterMap);
+ return clusterMap.sortByDistanceCost(client, nodes, nodes.size());
+ }
+
+ @Override
+ public List<? extends DatanodeDetails> sortDatanodesForWrite(
+ List<? extends DatanodeDetails> nodes, String clientMachine) {
+ Preconditions.checkArgument(!StringUtils.isEmpty(clientMachine),
clientMachine);
+ final NetworkTopology clusterMap;
+ try {
+ clusterMap = ozoneManager.getClusterMap();
+ } catch (NullPointerException ex) {
Review Comment:
- NullPointerException is supposed to indicate a bug and we should not catch
it. (e.g. it is a bug if ozoneManager is null but this change will hide the
bug.)
- Also creating exception is expensive.
Let's refactor the code:
```diff
+++
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/client/ScmTopologyClient.java
@@ -60,8 +59,7 @@ public ScmTopologyClient(
}
public NetworkTopology getClusterMap() {
- return requireNonNull(cache.get(),
- "ScmBlockLocationClient must have been initialized already.");
+ return cache.get();
}
```
```diff
+++
b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
@@ -1342,6 +1343,11 @@ public void setScmTopologyClient(
}
public NetworkTopology getClusterMap() {
+ return requireNonNull(scmTopologyClient.getClusterMap(),
+ "ScmBlockLocationClient must have been initialized already.");
+ }
+
+ public NetworkTopology getClusterMapAllowNull() {
return scmTopologyClient.getClusterMap();
}
```
--
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]
