You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "virajjasani (via GitHub)" <gi...@apache.org> on 2023/05/08 20:40:15 UTC
[GitHub] [hadoop] virajjasani commented on a diff in pull request #5554: HDFS-16978. RBF: Admin command to support bulk add of mount points
virajjasani commented on code in PR #5554:
URL: https://github.com/apache/hadoop/pull/5554#discussion_r1187807675
##########
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:
Good point!
For admin impl, it is not a big deal to cherry-pick on what response state store APIs support, however, as of today, putAll() at state store level does not support returning failed entries. we can take this up as a follow-up PR, so that this PR does not have to deal with all state store impl level changes (just to keep it clean). hence, let me take this up as a follow-up jira, sounds good to you?
##########
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:
makes sense, but we already support same dest nsid for multiple dest path (ns0->/path1 and ns1->/path1) as part of `-add` right? hence i was trying to keep that behavior for `-addAll` as well.
when we update single add behavior to support comma separated dest path as well (ns0->/path1 and ns1->/path2), at that time we can make change here to make it applicable for both, wdyt?
also, for single add, if we provide multiple nsid and same dest path, it still internally results into single add entry to mount table. it's just that when we finally start supporting multiple nsid and multiple dest path as part of single add command, making change to this area might be more impactful?
##########
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:
At high level, `dest.length != 1` still seems good for `-addAll` but perhaps restricting it with `dest.length !=nss.length` is maybe not needed as such IMO
--
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