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 "ayushtkn (via GitHub)" <gi...@apache.org> on 2023/05/24 04:08:35 UTC
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #4990: HDFS-13507. RBF RouterAdmin disable update functionality in add cmd and supports add/update one mount table with different destination
ayushtkn commented on code in PR #4990:
URL: https://github.com/apache/hadoop/pull/4990#discussion_r1203379687
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterAdminServer.java:
##########
@@ -425,6 +426,18 @@ private void verifyFileExistenceInDest(MountTable mountTable) throws IOException
}
}
+ private void verifyMountTableExistence(MountTable mountTable)
Review Comment:
Add a javadoc
there is a method already in class ``namespaceExists`` so can we do something like, ``VerifyMountEntryExists`` or something like that
##########
hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HDFSCommands.md:
##########
@@ -463,24 +464,24 @@ Usage:
[-refreshSuperUserGroupsConfiguration]
[-refreshCallQueue]
-| COMMAND\_OPTION | Description |
-|:---- |:---- |
-| `-add` *source* *nameservices* *destination* | Add a mount table entry or update if it exists. |
-| `-update` *source* *nameservices* *destination* | Update a mount table entry attributes. |
-| `-rm` *source* | Remove mount point of specified path. |
-| `-ls` `[-d]` *path* | List mount points under specified path. Specify -d parameter to get detailed listing.|
-| `-getDestination` *path* | Get the subcluster where a file is or should be created. |
-| `-setQuota` *path* `-nsQuota` *nsQuota* `-ssQuota` *ssQuota* | Set quota for specified path. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
-| `-setStorageTypeQuota` *path* `-storageType` *storageType* *stQuota* | Set storage type quota for specified path. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
-| `-clrQuota` *path* | Clear quota of given mount point. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
-| `-clrStorageTypeQuota` *path* | Clear storage type quota of given mount point. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
-| `-safemode` `enter` `leave` `get` | Manually set the Router entering or leaving safe mode. The option *get* will be used for verifying if the Router is in safe mode state. |
-| `-nameservice` `disable` `enable` *nameservice* | Disable/enable a name service from the federation. If disabled, requests will not go to that name service. |
-| `-getDisabledNameservices` | Get the name services that are disabled in the federation. |
-| `-refresh` | Update mount table cache of the connected router. |
+| COMMAND\_OPTION | Description |
+|:---- |:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `-add` *source* *nameservices* *destination* | Add a mount table entry. |
+| `-update` *source* *nameservices* *destination* | Update a mount table entry attributes. |
+| `-rm` *source* | Remove mount point of specified path. |
+| `-ls` `[-d]` *path* | List mount points under specified path. Specify -d parameter to get detailed listing. |
+| `-getDestination` *path* | Get the subcluster where a file is or should be created. |
+| `-setQuota` *path* `-nsQuota` *nsQuota* `-ssQuota` *ssQuota* | Set quota for specified path. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
+| `-setStorageTypeQuota` *path* `-storageType` *storageType* *stQuota* | Set storage type quota for specified path. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
+| `-clrQuota` *path* | Clear quota of given mount point. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
+| `-clrStorageTypeQuota` *path* | Clear storage type quota of given mount point. See [HDFS Quotas Guide](./HdfsQuotaAdminGuide.html) for the quota detail. |
Review Comment:
this seems some formatting change, can you avoid this to keep the code change here minimal?
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -699,32 +697,46 @@ public boolean addMount(AddMountAttributes addMountAttributes)
// Get the existing entry
MountTableManager mountTable = client.getMountTableManager();
MountTable existingEntry = getMountEntry(mount, mountTable);
- MountTable existingOrNewEntry =
- addMountAttributes.getNewOrUpdatedMountTableEntryWithAttributes(existingEntry);
- if (existingOrNewEntry == null) {
+ if (existingEntry != null) {
+ System.err.println("MountTable " + mount + " already exists.");
return false;
}
- if (existingEntry == null) {
- AddMountTableEntryRequest request = AddMountTableEntryRequest
- .newInstance(existingOrNewEntry);
- AddMountTableEntryResponse addResponse = mountTable.addMountTableEntry(request);
- boolean added = addResponse.getStatus();
- if (!added) {
- System.err.println("Cannot add mount point " + mount);
- }
- return added;
- } else {
- UpdateMountTableEntryRequest updateRequest =
- UpdateMountTableEntryRequest.newInstance(existingOrNewEntry);
- UpdateMountTableEntryResponse updateResponse =
- mountTable.updateMountTableEntry(updateRequest);
- boolean updated = updateResponse.getStatus();
- if (!updated) {
- System.err.println("Cannot update mount point " + mount);
- }
- return updated;
+ MountTable mountEntry = addMountAttributes.getMountTableEntryWithAttributes();
+ AddMountTableEntryRequest request = AddMountTableEntryRequest.newInstance(mountEntry);
+ AddMountTableEntryResponse addResponse = mountTable.addMountTableEntry(request);
+ boolean added = addResponse.getStatus();
+ if (!added) {
+ System.err.println("Cannot add mount point " + mount);
}
+ return added;
+ }
+
+ /**
+ * Return the map from namespace to destination.
+ * @param nss input namespaces.
+ * @param destinations input destinations.
+ * @return one map from namespace to destination.
+ */
+ public static Map<String, String> getDestMap(String[] nss, String[] destinations) {
+ assert destinations.length == 1 || destinations.length == nss.length;
+ Map<String, String> destMap = new LinkedHashMap<>();
+ boolean multiDest = destinations.length > 1;
+ for (int nsIndex = 0; nsIndex < nss.length; nsIndex++) {
+ int destIndex = multiDest ? nsIndex : 0;
+ destMap.put(nss[nsIndex], destinations[destIndex]);
+ }
+ return destMap;
+ }
+
+ private boolean isInvalidDestinations(String[] nss, String[] destinations) {
+ if (destinations.length > 1 && nss.length != destinations.length) {
+ String message = "Invalid namespaces and destinations. The number of destinations "
+ + destinations.length + " is not matched with the number of namespaces " + nss.length;
+ System.err.println(message);
Review Comment:
Invalid number of namespaces and destinations. The number of destinations: "
+ destinations.length + " is not equal to the number of namespaces: " + nss.length;
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -699,32 +697,46 @@ public boolean addMount(AddMountAttributes addMountAttributes)
// Get the existing entry
MountTableManager mountTable = client.getMountTableManager();
MountTable existingEntry = getMountEntry(mount, mountTable);
- MountTable existingOrNewEntry =
- addMountAttributes.getNewOrUpdatedMountTableEntryWithAttributes(existingEntry);
- if (existingOrNewEntry == null) {
+ if (existingEntry != null) {
+ System.err.println("MountTable " + mount + " already exists.");
return false;
}
- if (existingEntry == null) {
- AddMountTableEntryRequest request = AddMountTableEntryRequest
- .newInstance(existingOrNewEntry);
- AddMountTableEntryResponse addResponse = mountTable.addMountTableEntry(request);
- boolean added = addResponse.getStatus();
- if (!added) {
- System.err.println("Cannot add mount point " + mount);
- }
- return added;
- } else {
- UpdateMountTableEntryRequest updateRequest =
- UpdateMountTableEntryRequest.newInstance(existingOrNewEntry);
- UpdateMountTableEntryResponse updateResponse =
- mountTable.updateMountTableEntry(updateRequest);
- boolean updated = updateResponse.getStatus();
- if (!updated) {
- System.err.println("Cannot update mount point " + mount);
- }
- return updated;
+ MountTable mountEntry = addMountAttributes.getMountTableEntryWithAttributes();
+ AddMountTableEntryRequest request = AddMountTableEntryRequest.newInstance(mountEntry);
+ AddMountTableEntryResponse addResponse = mountTable.addMountTableEntry(request);
+ boolean added = addResponse.getStatus();
+ if (!added) {
+ System.err.println("Cannot add mount point " + mount);
}
+ return added;
+ }
+
+ /**
+ * Return the map from namespace to destination.
+ * @param nss input namespaces.
+ * @param destinations input destinations.
+ * @return one map from namespace to destination.
+ */
+ public static Map<String, String> getDestMap(String[] nss, String[] destinations) {
+ assert destinations.length == 1 || destinations.length == nss.length;
Review Comment:
I would say rather than the assert use the if else and throw a proper message, ``isInvalidDestinations(String[] nss, String[] destinations)`` can be refactored for it, or pass additional ``boolean`` there to specify if we need to do a print or not
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -162,23 +162,29 @@ private static String getUsage(String cmd) {
return usage.toString();
}
if (cmd.equals("-add")) {
- return "\t[-add <source> <nameservice1, nameservice2, ...> <destination> "
+ return "\t[-add <source> <nameservice1, nameservice2, ...> "
+ + "<one destination or the same number of destinations as nameservices> "
+ "[-readonly] [-faulttolerant] "
+ "[-order HASH|LOCAL|RANDOM|HASH_ALL|SPACE] "
+ "-owner <owner> -group <group> -mode <mode>]";
} else if (cmd.equals(ADD_ALL_COMMAND)) {
return "\t[" + ADD_ALL_COMMAND + " "
- + "<source1> <nameservice1,nameservice2,...> <destination1> "
- + "[-readonly] [-faulttolerant] " + "[-order HASH|LOCAL|RANDOM|HASH_ALL|SPACE] "
+ + "<source1> <nameservice1,nameservice2,...> "
Review Comment:
nit:
space after comma ``nameservice1, nameservice2``
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -699,32 +697,46 @@ public boolean addMount(AddMountAttributes addMountAttributes)
// Get the existing entry
MountTableManager mountTable = client.getMountTableManager();
MountTable existingEntry = getMountEntry(mount, mountTable);
- MountTable existingOrNewEntry =
- addMountAttributes.getNewOrUpdatedMountTableEntryWithAttributes(existingEntry);
- if (existingOrNewEntry == null) {
+ if (existingEntry != null) {
+ System.err.println("MountTable " + mount + " already exists.");
Review Comment:
``MountTable entry: + mount + already exists``
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java:
##########
@@ -1925,6 +1924,43 @@ public void testAddMultipleMountPointsFailure() throws Exception {
err.toString().contains("Cannot add mount points: [/testAddMultiMountPoints-01"));
}
+ @Test
+ public void testMountPointWithDifferentDestinations() throws Exception {
+ System.setOut(new PrintStream(out));
+ System.setErr(new PrintStream(err));
+
+ stateStore.loadCache(MountTableStoreImpl.class, true);
+ String[] argv = new String[] {"-add", "/multipleDestination0", "ns01,ns02", "/tmp0"};
+ assertEquals(0, ToolRunner.run(admin, argv));
+ err.reset();
+
+ stateStore.loadCache(MountTableStoreImpl.class, true);
+ argv = new String[] {"-add", "/multipleDestination1", "ns01,ns02", "/tmp1_1,/tmp2_1"};
+ assertEquals(0, ToolRunner.run(admin, argv));
+ err.reset();
+
+ argv = new String[] {"-add", "/multipleDestination2", "ns01", "/tmp1_2,/tmp2_2"};
+ assertEquals(-1, ToolRunner.run(admin, argv));
+ assertTrue(err.toString(), err.toString().contains("Invalid namespaces and destinations."));
+ err.reset();
+
+ argv = new String[] {"-add", "/multipleDestination2", "ns01,ns02", "/tmp1_2,/tmp2_2,/tmp3_2"};
+ assertEquals(-1, ToolRunner.run(admin, argv));
+ assertTrue(err.toString(), err.toString().contains("Invalid namespaces and destinations."));
+ err.reset();
+
+ System.setErr(new PrintStream(err));
+ stateStore.loadCache(MountTableStoreImpl.class, true);
+ argv = new String[] {"-update", "/multipleDestination0", "ns0,ns1", "/tmp0_0,/tmp1_0"};
+ assertEquals(0, ToolRunner.run(admin, argv));
+ err.reset();
+
+ stateStore.loadCache(MountTableStoreImpl.class, true);
+ argv = new String[] {"-update", "/multipleDestination1", "ns01,ns02", "/tmp1"};
+ assertEquals(0, ToolRunner.run(admin, argv));
+ err.reset();
Review Comment:
Add a couple of test cases for -addAll as well
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestRouterAdminCLI.java:
##########
@@ -1925,6 +1924,43 @@ public void testAddMultipleMountPointsFailure() throws Exception {
err.toString().contains("Cannot add mount points: [/testAddMultiMountPoints-01"));
}
+ @Test
+ public void testMountPointWithDifferentDestinations() throws Exception {
+ System.setOut(new PrintStream(out));
+ System.setErr(new PrintStream(err));
+
+ stateStore.loadCache(MountTableStoreImpl.class, true);
+ String[] argv = new String[] {"-add", "/multipleDestination0", "ns01,ns02", "/tmp0"};
Review Comment:
check if ns01, ns02 works or not, else while getting the ns we should trim as well in the prod code
--
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