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: [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]
