From 887ff39b306dc21883b0863fb0e5f8cfeb63f2ee Mon Sep 17 00:00:00 2001 From: zhaoym6 Date: Thu, 28 May 2020 15:07:08 +0800 Subject: [PATCH 1/4] HDFS-14984. HDFS setQuota: Error message should be added for invalid input max range value to hdfs dfsadmin -setQuota command --- .../org/apache/hadoop/hdfs/DFSClient.java | 19 ++--- .../apache/hadoop/hdfs/tools/DFSAdmin.java | 20 ++++- .../org/apache/hadoop/hdfs/TestQuota.java | 73 +++++++++++++++++++ 3 files changed, 101 insertions(+), 11 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java index 72b2113943756..5da1512eb3e94 100755 --- a/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java +++ b/hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java @@ -2553,16 +2553,17 @@ void setQuota(String src, long namespaceQuota, long storagespaceQuota) throws IOException { checkOpen(); // sanity check - if ((namespaceQuota <= 0 && - namespaceQuota != HdfsConstants.QUOTA_DONT_SET && - namespaceQuota != HdfsConstants.QUOTA_RESET) || - (storagespaceQuota < 0 && + if (namespaceQuota <= 0 && + namespaceQuota != HdfsConstants.QUOTA_DONT_SET && + namespaceQuota != HdfsConstants.QUOTA_RESET){ + throw new IllegalArgumentException("Invalid values for " + + "namespace quota : " + namespaceQuota); + } + if (storagespaceQuota < 0 && storagespaceQuota != HdfsConstants.QUOTA_DONT_SET && - storagespaceQuota != HdfsConstants.QUOTA_RESET)) { - throw new IllegalArgumentException("Invalid values for quota : " + - namespaceQuota + " and " + - storagespaceQuota); - + storagespaceQuota != HdfsConstants.QUOTA_RESET) { + throw new IllegalArgumentException("Invalid values for " + + "storagespace quota : " + storagespaceQuota); } try (TraceScope ignored = newPathTraceScope("setQuota", src)) { // Pass null as storage type for traditional namespace/storagespace quota. diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java index 04960e3c3e2ce..c2a0efdc7d3c3 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java @@ -201,8 +201,19 @@ private static class SetQuotaCommand extends DFSAdminCommand { super(conf); CommandFormat c = new CommandFormat(2, Integer.MAX_VALUE); List parameters = c.parse(args, pos); - this.quota = - StringUtils.TraditionalBinaryPrefix.string2long(parameters.remove(0)); + String str = parameters.get(0).trim(); + try { + this.quota = StringUtils.TraditionalBinaryPrefix + .string2long(parameters.remove(0)); + } catch(NumberFormatException e){ + throw new IllegalArgumentException("\"" + str + + "\" is not a valid value for a quota."); + } + if (HdfsConstants.QUOTA_DONT_SET == this.quota) { + System.out.print("WARN: \"" + this.quota + + "\" means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values. \n"); + } this.args = parameters.toArray(new String[parameters.size()]); } @@ -323,6 +334,11 @@ private static class SetSpaceQuotaCommand extends DFSAdminCommand { } catch (NumberFormatException nfe) { throw new IllegalArgumentException("\"" + str + "\" is not a valid value for a quota."); } + if (HdfsConstants.QUOTA_DONT_SET == quota) { + System.out.print("WARN: \"" + this.quota + + "\" means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values. \n"); + } String storageTypeString = StringUtils.popOptionWithArgument("-storageType", parameters); if (storageTypeString != null) { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java index 7754b53653cf4..cb04a8157f359 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java @@ -1216,6 +1216,79 @@ private void compareQuotaUsage(final QuotaUsage fromContentSummary, assertEquals(fromContentSummary, quotaUsage); } + /** + * Test to set quote with warn msg. + */ + @Test(timeout = 30000) + public void testSetQuotaWarningMsg() throws Exception { + + final DFSAdmin dfsAdmin = new DFSAdmin(conf); + final Path dir = new Path( + PathUtils.getTestDir(getClass()).getPath(), + GenericTestUtils.getMethodName()); + assertTrue(dfs.mkdirs(dir)); + + final List outs = Lists.newArrayList(); + + /* set quota HdfsConstants.QUOTA_DONT_SET */ + resetStream(); + outs.clear(); + final int ret = ToolRunner.run( + dfsAdmin, + new String[] {"-setQuota", + String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); + assertEquals(0, ret); + scanIntoList(OUT_STREAM, outs); + assertEquals(1, outs.size()); + assertThat(outs.get(0), + is(allOf(containsString("WARN:"), + containsString("means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values.")))); + + final List outs1 = Lists.newArrayList(); + /* set quota 0 */ + resetStream(); + outs1.clear(); + final int ret1 = ToolRunner.run( + dfsAdmin, + new String[] {"-setQuota", "0", dir.toString()}); + assertEquals(-1, ret1); + scanIntoList(ERR_STREAM, outs1); + assertEquals(2, outs1.size()); + assertThat(outs1.get(0), + is(allOf(containsString("setQuota"), + containsString("Invalid values for namespace quota")))); + } + + /** + * Test to set quote with warn msg. + */ + @Test(timeout = 30000) + public void testSetSpaceQuotaWarningMsg() throws Exception { + + final DFSAdmin dfsAdmin = new DFSAdmin(conf); + final Path dir = new Path( + PathUtils.getTestDir(getClass()).getPath(), + GenericTestUtils.getMethodName()); + assertTrue(dfs.mkdirs(dir)); + + final List outs = Lists.newArrayList(); + + /* set quota HdfsConstants.QUOTA_DONT_SET */ + resetStream(); + outs.clear(); + final int ret = ToolRunner.run( + dfsAdmin, + new String[] {"-setSpaceQuota", + String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); + assertEquals(0, ret); + scanIntoList(OUT_STREAM, outs); + assertEquals(1, outs.size()); + assertThat(outs.get(0), + is(allOf(containsString("WARN:"), + containsString("means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values.")))); + } /** * Test to set space quote using negative number. From 3c4f9ee8f1dd1163a5b9b49e71725383e50ab373 Mon Sep 17 00:00:00 2001 From: zhaoym6 Date: Mon, 6 Jul 2020 12:01:25 +0800 Subject: [PATCH 2/4] use System.err.print for the warning message --- .../src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java index c2a0efdc7d3c3..310513a44de7c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java @@ -210,7 +210,7 @@ private static class SetQuotaCommand extends DFSAdminCommand { "\" is not a valid value for a quota."); } if (HdfsConstants.QUOTA_DONT_SET == this.quota) { - System.out.print("WARN: \"" + this.quota + + System.err.print("WARN: \"" + this.quota + "\" means QUOTA_DONT_SET, quota will not be set, " + "it keep the old values. \n"); } @@ -335,7 +335,7 @@ private static class SetSpaceQuotaCommand extends DFSAdminCommand { throw new IllegalArgumentException("\"" + str + "\" is not a valid value for a quota."); } if (HdfsConstants.QUOTA_DONT_SET == quota) { - System.out.print("WARN: \"" + this.quota + + System.err.print("WARN: \"" + this.quota + "\" means QUOTA_DONT_SET, quota will not be set, " + "it keep the old values. \n"); } From a2dbe5df808924569aae156e533ada516c2dca2a Mon Sep 17 00:00:00 2001 From: zhaoym6 Date: Mon, 6 Jul 2020 15:46:57 +0800 Subject: [PATCH 3/4] update UT to change err stream --- .../src/test/java/org/apache/hadoop/hdfs/TestQuota.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java index cb04a8157f359..3b3ab4b1468d2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java @@ -1238,7 +1238,7 @@ public void testSetQuotaWarningMsg() throws Exception { new String[] {"-setQuota", String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); assertEquals(0, ret); - scanIntoList(OUT_STREAM, outs); + scanIntoList(ERR_STREAM, outs); assertEquals(1, outs.size()); assertThat(outs.get(0), is(allOf(containsString("WARN:"), @@ -1282,7 +1282,7 @@ public void testSetSpaceQuotaWarningMsg() throws Exception { new String[] {"-setSpaceQuota", String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); assertEquals(0, ret); - scanIntoList(OUT_STREAM, outs); + scanIntoList(ERR_STREAM, outs); assertEquals(1, outs.size()); assertThat(outs.get(0), is(allOf(containsString("WARN:"), From 597aeb1fba4ad99624c566662586e15924d7c03e Mon Sep 17 00:00:00 2001 From: zhaoym6 Date: Tue, 7 Jul 2020 10:53:57 +0800 Subject: [PATCH 4/4] fix style and throw exception instead of print --- .../apache/hadoop/hdfs/tools/DFSAdmin.java | 12 +++++----- .../org/apache/hadoop/hdfs/TestQuota.java | 22 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java index 310513a44de7c..15d9d4cf8d423 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java @@ -210,9 +210,9 @@ private static class SetQuotaCommand extends DFSAdminCommand { "\" is not a valid value for a quota."); } if (HdfsConstants.QUOTA_DONT_SET == this.quota) { - System.err.print("WARN: \"" + this.quota + - "\" means QUOTA_DONT_SET, quota will not be set, " - + "it keep the old values. \n"); + throw new IllegalArgumentException("WARN: \"" + this.quota + + "\" means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values."); } this.args = parameters.toArray(new String[parameters.size()]); } @@ -335,9 +335,9 @@ private static class SetSpaceQuotaCommand extends DFSAdminCommand { throw new IllegalArgumentException("\"" + str + "\" is not a valid value for a quota."); } if (HdfsConstants.QUOTA_DONT_SET == quota) { - System.err.print("WARN: \"" + this.quota + - "\" means QUOTA_DONT_SET, quota will not be set, " - + "it keep the old values. \n"); + throw new IllegalArgumentException("WARN: \"" + this.quota + + "\" means QUOTA_DONT_SET, quota will not be set, " + + "it keep the old values."); } String storageTypeString = StringUtils.popOptionWithArgument("-storageType", parameters); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java index 3b3ab4b1468d2..65ec8b3e189b4 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java @@ -1236,14 +1236,15 @@ public void testSetQuotaWarningMsg() throws Exception { final int ret = ToolRunner.run( dfsAdmin, new String[] {"-setQuota", - String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); - assertEquals(0, ret); + String.valueOf(HdfsConstants.QUOTA_DONT_SET), + dir.toString()}); + assertEquals(-1, ret); scanIntoList(ERR_STREAM, outs); - assertEquals(1, outs.size()); + assertEquals(2, outs.size()); assertThat(outs.get(0), is(allOf(containsString("WARN:"), - containsString("means QUOTA_DONT_SET, quota will not be set, " + - "it keep the old values.")))); + containsString("means QUOTA_DONT_SET, quota will " + + "not be set, it keep the old values.")))); final List outs1 = Lists.newArrayList(); /* set quota 0 */ @@ -1280,14 +1281,15 @@ public void testSetSpaceQuotaWarningMsg() throws Exception { final int ret = ToolRunner.run( dfsAdmin, new String[] {"-setSpaceQuota", - String.valueOf(HdfsConstants.QUOTA_DONT_SET), dir.toString()}); - assertEquals(0, ret); + String.valueOf(HdfsConstants.QUOTA_DONT_SET), + dir.toString()}); + assertEquals(-1, ret); scanIntoList(ERR_STREAM, outs); - assertEquals(1, outs.size()); + assertEquals(2, outs.size()); assertThat(outs.get(0), is(allOf(containsString("WARN:"), - containsString("means QUOTA_DONT_SET, quota will not be set, " + - "it keep the old values.")))); + containsString("means QUOTA_DONT_SET, quota will " + + "not be set, it keep the old values.")))); } /**