[GitHub] [ozone] adoroszlai commented on a diff in pull request #3835: HDDS-7329. Extend ozone admin datanode usageinfo and list info to accept hostname parameter

[GitBox <gi...@apache.org>]

adoroszlai commented on code in PR #3835:
URL: https://github.com/apache/ozone/pull/3835#discussion_r996334888


##########
hadoop-hdds/interface-admin/src/main/resources/proto.lock:
##########
@@ -888,6 +888,11 @@
                 "id": 4,
                 "name": "count",
                 "type": "uint32"
+              },
+              {
+                "id": 5,
+                "name": "hostname",
+                "type": "string"

Review Comment:
   Please don't change `proto.lock`.  This file is needed to allow checking backwards compatibility.  If we change both proto definitions and the lock file in tandem, the check cannot flag incompatible changes.



##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/node/TestSCMNodeManager.java:
##########
@@ -1704,7 +1703,7 @@ public void testScmRegisterNodeWithIpAddress()
   @Test
   public void testScmRegisterNodeWithHostname()
       throws IOException, InterruptedException, AuthenticationException {
-    testScmRegisterNodeWithNetworkTopology(true);
+    testScmRegisterNodeWithNetworkTopology();
   }

Review Comment:
   These tests used to exercise the same use case with different config setting (use hostname true/false).  Since the config is being removed, they are now duplicates.  Calling the same underlying method from two different test methods without any difference is unnecessary.  Please remove the methods `...WithIpAddress` and `...WithHostname`, move the `@Test` annotation to the delegate methods, and remove the `private` modifier from those methods.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -365,21 +363,14 @@ public RegisteredCommand register(
     InetAddress dnAddress = Server.getRemoteIp();
     if (dnAddress != null) {
       // Mostly called inside an RPC, update ip
-      if (!useHostname) {
-        datanodeDetails.setHostName(dnAddress.getHostName());
-      }
+      datanodeDetails.setHostName(dnAddress.getHostName());
       datanodeDetails.setIpAddress(dnAddress.getHostAddress());
     }
 
-    String dnsName;
-    String networkLocation;
+    String dnsName = datanodeDetails.getIpAddress();
+    String hostName = datanodeDetails.getHostName();

Review Comment:
   `dnsName` and `hostName` roughly mean the same thing.  Please use `ipAddress` for IP address.



##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/protocol/StorageContainerLocationProtocol.java:
##########
@@ -386,18 +386,20 @@ StartContainerBalancerResponseProto startContainerBalancer(
   boolean getContainerBalancerStatus() throws IOException;
 
   /**
-   * Get Datanode usage information by ip or uuid.
+   * Get Datanode usage information by ip or uuid or hostname.
    *
    * @param ipaddress datanode IP address String
    * @param uuid datanode UUID String
+   * @param hostname datanode hostname address String
    * @param clientVersion Client's version number
    * @return List of DatanodeUsageInfoProto. Each element contains info such as
    * capacity, SCMused, and remaining space.
    * @throws IOException
    * @see org.apache.hadoop.ozone.ClientVersion
    */
   List<HddsProtos.DatanodeUsageInfoProto> getDatanodeUsageInfo(
-      String ipaddress, String uuid, int clientVersion) throws IOException;
+      String ipaddress, String uuid,
+      String hostname, int clientVersion) throws IOException;

Review Comment:
   Please also keep the method with existing signature for backward compatibility.  I think it can be a `default` method calling the new one, passing `null` for `hostname`.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -121,9 +121,10 @@ public class SCMNodeManager implements NodeManager {
   private final SCMStorageConfig scmStorageConfig;
   private final NetworkTopology clusterMap;
   private final DNSToSwitchMapping dnsToSwitchMapping;
-  private final boolean useHostname;
   private final ConcurrentHashMap<String, Set<String>> dnsToUuidMap =
       new ConcurrentHashMap<>();
+  private final ConcurrentHashMap<String, Set<String>> hostNmToUuidMap =

Review Comment:
   Prefer interface for variable type and please avoid obscure abbreviation that only saves 2 characters:
   
   ```suggestion
     private final Map<String, Set<String>> hostNameToUuidMap =
   ```



##########
hadoop-hdds/tools/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/ListInfoSubcommand.java:
##########
@@ -46,11 +46,16 @@ public class ListInfoSubcommand extends ScmSubcommand {
       defaultValue = "")
   private String ipaddress;
 
-  @CommandLine.Option(names = {"--id"},
+  @CommandLine.Option(names = {"--uuid"},

Review Comment:
   Please keep the old name as an alias for backwards compatibility.



##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/node/SCMNodeManager.java:
##########
@@ -468,6 +458,17 @@ private synchronized void addEntryToDnsToUuidMap(
     dnList.add(uuid);
   }
 
+  @SuppressFBWarnings(value = "AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION")
+  private synchronized void addEntryToHostNmToUuidMap(
+      String hostName, String uuid) {
+    Set<String> dnList = hostNmToUuidMap.get(hostName);
+    if (dnList == null) {
+      dnList = ConcurrentHashMap.newKeySet();
+      hostNmToUuidMap.put(hostName, dnList);
+    }
+    dnList.add(uuid);

Review Comment:
   We can simplify the code and avoid having to add a suppression:
   
   ```suggestion
     private void addEntryToHostNameToUuidMap(
         String hostName, String uuid) {
       hostNmToUuidMap
           .computeIfAbsent(hostName, any -> ConcurrentHashMap.newKeySet())
           .add(uuid);
   ```



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org