Skip to content

HDFS-14984. HDFS setQuota: Error message should be added for invalid … #2037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
zhaoyim wants to merge 4 commits into from
Closed

HDFS-14984. HDFS setQuota: Error message should be added for invalid … #2037

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
887ff39
HDFS-14984. HDFS setQuota: Error message should be added for invalid …
zhaoyim May 28, 2020
3c4f9ee
use System.err.print for the warning message
zhaoyim Jul 6, 2020
a2dbe5d
update UT to change err stream
zhaoyim Jul 6, 2020
597aeb1
fix style and throw exception instead of print
zhaoyim Jul 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this condition statement should be if (storagesapceQuoat <= 0)?

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hexiaoqiao Thanks for review! (-9223372036854775808L - 1) == Long.MAX_VALUE and QUOTA_RESET = -1L so I think it's better to check more conditions.

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if we should update storagespaceQuota < 0 to storagespaceQuota <= 0, rather than whole condition statement.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see. If so, seems the -1L will throw the exception, but it should be OK for the internal use, such as clear quota.

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhaoyim for your explanation. But I am still confused why this condition is different with namespacequota? Sorry I do not find user manuals about set*Quota.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hexiaoqiao In my understand, the diff is after created a dir, the name quota is already 1 and the name quota count it self, but the space quota can be 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.
Expand Down
20 changes: 18 additions & 2 deletions hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,19 @@ private static class SetQuotaCommand extends DFSAdminCommand {
super(conf);
CommandFormat c = new CommandFormat(2, Integer.MAX_VALUE);
List<String> 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 +
Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it should be using System.err.print.
Another side, it is better to throw exception and no need to execute the following logic if meet invalid parameter.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to System.err.print. And I agree with you, not need to execute the backward code, but here
my confusion is this message just a warning message, it is suitable to throw an Exception ? Any ideas? Thanks!

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or how about return directly? I think it is not necessary to send RPC request to NameNode if has checked the parameter is invalid.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried return directly before, but there is an extra invalid message throw setQuota: null. checked the code it is not easy to change, because the warn message is in the SetSpaceQuotaCommand constructor. Maybe throw an exception or current solution are both OK. any ideas? Thanks!

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that I think it is OK to throw exception that avoid send RPC request to NameNode if parameter is invalid which client has checked.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with U, change to throw exception. Thanks for confirm!

"\" means QUOTA_DONT_SET, quota will not be set, "
+ "it keep the old values. \n");
}
this.args = parameters.toArray(new String[parameters.size()]);
}

Expand Down Expand Up @@ -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 +
Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Jul 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same as last comment.

Copy link
Contributor Author

@zhaoyim zhaoyim Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to System.err.print

"\" means QUOTA_DONT_SET, quota will not be set, "
+ "it keep the old values. \n");
}
String storageTypeString =
StringUtils.popOptionWithArgument("-storageType", parameters);
if (storageTypeString != null) {
Expand Down
73 changes: 73 additions & 0 deletions hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestQuota.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> 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<String> 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<String> 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.
Expand Down