[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5554: HDFS-16978. RBF: Admin command to support bulk add of mount points

["ayushtkn (via GitHub)" <gi...@apache.org>]

ayushtkn commented on code in PR #5554:
URL: https://github.com/apache/hadoop/pull/5554#discussion_r1187759979


##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java:
##########
@@ -1840,6 +1846,46 @@ public void testDumpState() throws Exception {
         buffer.toString().trim().replaceAll("[0-9]{4,}+", "XXX"));
   }
 
+  @Test
+  public void testAddMultipleMountPoints() throws Exception {
+    String[] argv =
+        new String[] {"-addAll", "/testAddMultipleMountPoints-01", "ns01", "/dest01", ",",
+            "/testAddMultipleMountPoints-02", "ns02,ns03", "/dest02", "-order", "HASH_ALL",
+            "-faulttolerant", ",", "/testAddMultipleMountPoints-03", "ns03", "/dest03"};
+    assertEquals(0, ToolRunner.run(admin, argv));
+
+    stateStore.loadCache(MountTableStoreImpl.class, true);
+    GetMountTableEntriesRequest request = GetMountTableEntriesRequest
+        .newInstance("/testAddMultipleMountPoints-01");
+    GetMountTableEntriesResponse response = client.getMountTableManager()
+        .getMountTableEntries(request);
+    assertEquals(1, response.getEntries().size());
+    List<RemoteLocation> destinations = response.getEntries().get(0).getDestinations();
+    assertEquals(1, destinations.size());
+    assertEquals("/testAddMultipleMountPoints-01", destinations.get(0).getSrc());
+    assertEquals("/dest01", destinations.get(0).getDest());
+    assertEquals("ns01", destinations.get(0).getNameserviceId());
+
+    request = GetMountTableEntriesRequest.newInstance("/testAddMultipleMountPoints-02");
+    response = client.getMountTableManager().getMountTableEntries(request);
+    destinations = response.getEntries().get(0).getDestinations();
+    assertEquals(2, destinations.size());
+    assertEquals("/testAddMultipleMountPoints-02", destinations.get(0).getSrc());
+    assertEquals("/dest02", destinations.get(0).getDest());
+    assertEquals("ns02", destinations.get(0).getNameserviceId());
+    assertEquals("/testAddMultipleMountPoints-02", destinations.get(1).getSrc());
+    assertEquals("/dest02", destinations.get(1).getDest());
+    assertEquals("ns03", destinations.get(1).getNameserviceId());
+
+    request = GetMountTableEntriesRequest.newInstance("/testAddMultipleMountPoints-03");
+    response = client.getMountTableManager().getMountTableEntries(request);
+    destinations = response.getEntries().get(0).getDestinations();
+    assertEquals(1, destinations.size());
+    assertEquals("/testAddMultipleMountPoints-03", destinations.get(0).getSrc());
+    assertEquals("/dest03", destinations.get(0).getDest());
+    assertEquals("ns03", destinations.get(0).getNameserviceId());

Review Comment:
   This validation is done 3 times, can refactor and have a util to do that. The test can be changed like this(format it)
   
   ```
    public void testAddMultipleMountPoints() throws Exception {
       String[] argv =
           new String[] {"-addAll", "/testAddMultipleMountPoints-01/dir1", "ns01", "/dest01", ",",
               "/testAddMultipleMountPoints-01", "ns02,ns03", "/dest02", "-order", "HASH_ALL",
               "-faulttolerant", ",", "/testAddMultipleMountPoints-03", "ns03", "/dest03", ",", "/testAddMultipleMountPoints-01/dir1", "ns01", "/dest02", ",",};
       assertEquals(0, ToolRunner.run(admin, argv));
   
       stateStore.loadCache(MountTableStoreImpl.class, true);
       validateMountEntry("/testAddMultipleMountPoints-01/dir1", 1, new String[] {"/dest01"},
           new String[] {"ns01"});
       validateMountEntry("/testAddMultipleMountPoints-02", 2, new String[] {"/dest02", "/dest02"},
           new String[] {"ns02", "ns03"});
   
       validateMountEntry("/testAddMultipleMountPoints-03", 1, new String[] {"/dest03"},
           new String[] {"ns03"});
     }
   
     private static void validateMountEntry(String mountName, int numDest, String[] dest, String[] nss)
         throws IOException {
       GetMountTableEntriesRequest request = GetMountTableEntriesRequest.newInstance(mountName);
       GetMountTableEntriesResponse response =
           client.getMountTableManager().getMountTableEntries(request);
       assertEquals(1, response.getEntries().size());
       List<RemoteLocation> destinations = response.getEntries().get(0).getDestinations();
       assertEquals(numDest, destinations.size());
       for (int i = 0; i < numDest; i++) {
         assertEquals(mountName, destinations.get(i).getSrc());
         assertEquals(dest[i], destinations.get(i).getDest());
         assertEquals(nss[i], destinations.get(i).getNameserviceId());
       }
     }
   ```



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/AddMountAttributes.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdfs.tools.federation;
+
+import java.io.IOException;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+import org.apache.hadoop.hdfs.server.federation.resolver.order.DestinationOrder;
+import org.apache.hadoop.hdfs.server.federation.store.records.MountTable;
+
+/**
+ * Add mount entry attributes to be used by Router admin.
+ */
+public class AddMountAttributes {
+
+  private String mount;
+  private String[] nss;
+  private String dest;
+  private boolean readonly;
+  private boolean faultTolerant;
+  private DestinationOrder order;
+  private RouterAdmin.ACLEntity aclInfo;
+  private int paramIndex;
+
+  public String getMount() {
+    return mount;
+  }
+
+  public void setMount(String mount) {
+    this.mount = mount;
+  }
+
+  public String[] getNss() {
+    return nss;
+  }
+
+  public void setNss(String[] nss) {
+    this.nss = nss;
+  }
+
+  public String getDest() {
+    return dest;
+  }
+
+  public void setDest(String dest) {
+    this.dest = dest;
+  }
+
+  public boolean isReadonly() {
+    return readonly;
+  }
+
+  public void setReadonly(boolean readonly) {
+    this.readonly = readonly;
+  }
+
+  public boolean isFaultTolerant() {
+    return faultTolerant;
+  }
+
+  public void setFaultTolerant(boolean faultTolerant) {
+    this.faultTolerant = faultTolerant;
+  }
+
+  public DestinationOrder getOrder() {
+    return order;
+  }
+
+  public void setOrder(DestinationOrder order) {
+    this.order = order;
+  }
+
+  public RouterAdmin.ACLEntity getAclInfo() {
+    return aclInfo;
+  }
+
+  public void setAclInfo(RouterAdmin.ACLEntity aclInfo) {
+    this.aclInfo = aclInfo;
+  }
+
+  public int getParamIndex() {
+    return paramIndex;
+  }
+
+  public void setParamIndex(int paramIndex) {
+    this.paramIndex = paramIndex;
+  }
+
+  /**
+   * Retrieve mount table object with all attributes derived from this object.
+   *
+   * @return Mount table object with updated attributes.
+   * @throws IOException If mount table instantiation fails.
+   */
+  public MountTable getMountTableEntryWithAttributes() throws IOException {
+    String normalizedMount = RouterAdmin.normalizeFileSystemPath(this.getMount());
+    return getMountTableForAddRequest(normalizedMount);
+  }
+
+  /**
+   * Retrieve mount table object with all attributes derived from this object.
+   * The returned mount table could be either new or existing one with updated attributes.
+   *
+   * @param existingEntry Existing mount table entry. If null, new mount table object is created,
+   * otherwise the existing mount table object is updated.
+   * @return Mount table object with updated attributes.

Review Comment:
   nit:
   ``MountTable`` object 
   
   in the above method as well.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -462,6 +482,136 @@ public int run(String[] argv) throws Exception {
     return exitCode;
   }
 
+  /**
+   * Add all mount point entries provided in the request.
+   *
+   * @param parameters Parameters for the mount points.
+   * @param i Current index on the parameters array.
+   * @return True if adding all mount points was successful, False otherwise.
+   * @throws IOException If the RPC call to add the mount points fail.
+   */
+  private boolean addAllMount(String[] parameters, int i) throws IOException {
+    List<AddMountAttributes> addMountAttributesList = new ArrayList<>();
+    while (i < parameters.length) {
+      AddMountAttributes addMountAttributes = getAddMountAttributes(parameters, i, true);
+      if (addMountAttributes == null) {
+        return false;
+      }
+      i = addMountAttributes.getParamIndex();
+      addMountAttributesList.add(addMountAttributes);
+    }
+    List<MountTable> addEntries = getMountTablesFromAddAllAttributes(addMountAttributesList);
+    AddMountTableEntriesRequest request =
+        AddMountTableEntriesRequest.newInstance(addEntries);
+    MountTableManager mountTable = client.getMountTableManager();
+    AddMountTableEntriesResponse addResponse =
+        mountTable.addMountTableEntries(request);
+    boolean added = addResponse.getStatus();
+    if (!added) {
+      System.err.println("Cannot add some or all mount points");
+    }

Review Comment:
   How to figure out which one failed? So, it can be retried. Say you pushed for 1K entries and 998 got added, how to figure out which are the two failed, so we can retry.
   Validating against the ls mount and the script should too much effort here.



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -462,6 +482,136 @@ public int run(String[] argv) throws Exception {
     return exitCode;
   }
 
+  /**
+   * Add all mount point entries provided in the request.
+   *
+   * @param parameters Parameters for the mount points.
+   * @param i Current index on the parameters array.
+   * @return True if adding all mount points was successful, False otherwise.
+   * @throws IOException If the RPC call to add the mount points fail.
+   */
+  private boolean addAllMount(String[] parameters, int i) throws IOException {
+    List<AddMountAttributes> addMountAttributesList = new ArrayList<>();
+    while (i < parameters.length) {
+      AddMountAttributes addMountAttributes = getAddMountAttributes(parameters, i, true);
+      if (addMountAttributes == null) {
+        return false;
+      }
+      i = addMountAttributes.getParamIndex();
+      addMountAttributesList.add(addMountAttributes);
+    }
+    List<MountTable> addEntries = getMountTablesFromAddAllAttributes(addMountAttributesList);
+    AddMountTableEntriesRequest request =
+        AddMountTableEntriesRequest.newInstance(addEntries);
+    MountTableManager mountTable = client.getMountTableManager();
+    AddMountTableEntriesResponse addResponse =
+        mountTable.addMountTableEntries(request);
+    boolean added = addResponse.getStatus();
+    if (!added) {
+      System.err.println("Cannot add some or all mount points");
+    }
+    return added;
+  }
+
+  /**
+   * From the given params, form and retrieve AddMountAttributes object. This object is meant
+   * to be used while adding single or multiple mount points with their own specific attributes.
+   *
+   * @param parameters Parameters for the mount point.
+   * @param i Current index on the parameters array.
+   * @param isMultipleAdd True if multiple mount points are to be added, False if single mount
+   * point is to be added.
+   * @return AddMountAttributes object.
+   */
+  private static AddMountAttributes getAddMountAttributes(String[] parameters, int i,
+      boolean isMultipleAdd) {
+    // Mandatory parameters
+    String mount = parameters[i++];
+    String[] nss = parameters[i++].split(",");
+    String dest = parameters[i++];

Review Comment:
   This I think got pulled up from the Add command which is broken and getting fixed.
   As of now. If you want your targets to be like ns0 to /path1 and ns1 to /path2, you need to do an add and then call for an update, because add doesn't take multiple dest but only one. (There is PR to handle add)
   
   For AddAll, calling update for each of of the entry would be tough. Can you handle that for addAll.
   
   Should be easy go.
   ```
       String[] dest = parameters[i++].split(",");
       if(dest.length != 1 && dest.length !=nss.length) {
         throw new IllegalArgumentException("Number of destinations should be equal to number of namespace or one");
       }
   ```
   Adjust isMultipleAdd as well in this if check. Then when setting it can figure out if it is one or equal to NS and set in the Mount requests



##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java:
##########
@@ -1840,6 +1846,46 @@ public void testDumpState() throws Exception {
         buffer.toString().trim().replaceAll("[0-9]{4,}+", "XXX"));
   }
 
+  @Test
+  public void testAddMultipleMountPoints() throws Exception {

Review Comment:
   Add a failure case as well, where the addition fails, all the below three are success cases



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

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


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