HDFS-17017. Fix the issue of arguments number limit in report command in DFSAdmin #5667
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
The failed unit test seems unrelated to the change. |
Contributor
Author
|
Hi @Hexiaoqiao @ZanderXu @tomscut @ayushtkn please help review this minor changes when you are available, Thanks. |
ayushtkn
approved these changes
May 19, 2023
Member
ayushtkn
left a comment
ayushtkn
left a comment
There was a problem hiding this comment.
Thanx @haiyang1987 for the fix, I have triggered the build again. It was induced via HDFS-16521, I have linked that in the original ticket.
In case you find the numbers messed up for any of the other commands as well, can you raise a ticket to fix it as well?
virajjasani
approved these changes
May 19, 2023
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1 (non-binding), thanks for the fix @haiyang1987!
Contributor
|
Functionally this change makes sense because all of them are arguments, however in practice Thanks for attempting to fix the max argument @haiyang1987 |
Member
|
Answered on the ticket. The above doesn't hold true, it isn't intersection and a very valid use case and a miss on the actual PR. "Period" Good find @haiyang1987, I will merge it in a day, giving some time to @jojochuang if he feels otherwise. |
virajjasani
suggested changes
May 19, 2023
Contributor
There was a problem hiding this comment.
a miss on the actual PR. "Period"
I agree.
@haiyang1987 for this PR, since you already have the opportunity, I would like to propose these changes so that any new argument in future will not have to go through the same fate:
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 d717476dded..c25e2cf3579 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
@@ -489,7 +489,11 @@ public DFSAdmin(Configuration conf) {
protected DistributedFileSystem getDFS() throws IOException {
return AdminHelper.checkAndGetDFS(getFS(), getConf());
}
-
+
+ public static final String[] DFS_REPORT_ARGS =
+ new String[] {"-live", "-dead", "-decommissioning", "-enteringmaintenance", "-inmaintenance",
+ "-slownodes"};
+
/**
* Gives a report on how the FileSystem is doing.
* @exception IOException if the filesystem does not exist.
@@ -581,16 +585,16 @@ public void report(String[] argv, int i) throws IOException {
List<String> args = Arrays.asList(argv);
// Truncate already handled arguments before parsing report()-specific ones
args = new ArrayList<String>(args.subList(i, args.size()));
- final boolean listLive = StringUtils.popOption("-live", args);
- final boolean listDead = StringUtils.popOption("-dead", args);
+ final boolean listLive = StringUtils.popOption(DFS_REPORT_ARGS[0], args);
+ final boolean listDead = StringUtils.popOption(DFS_REPORT_ARGS[1], args);
final boolean listDecommissioning =
- StringUtils.popOption("-decommissioning", args);
+ StringUtils.popOption(DFS_REPORT_ARGS[2], args);
final boolean listEnteringMaintenance =
- StringUtils.popOption("-enteringmaintenance", args);
+ StringUtils.popOption(DFS_REPORT_ARGS[3], args);
final boolean listInMaintenance =
- StringUtils.popOption("-inmaintenance", args);
+ StringUtils.popOption(DFS_REPORT_ARGS[4], args);
final boolean listSlowNodes =
- StringUtils.popOption("-slownodes", args);
+ StringUtils.popOption(DFS_REPORT_ARGS[5], args);
// If no filter flags are found, then list all DN types
@@ -2399,7 +2403,7 @@ public int run(String[] argv) {
return exitCode;
}
} else if ("-report".equals(cmd)) {
- if (argv.length > 6) {
+ if (argv.length > 7) {
printUsage(cmd);
return exitCode;
}
diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java
index d81aebf3c2e..eaa7a88ca0d 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/TestDFSAdmin.java
@@ -795,6 +795,16 @@ public void testReportCommand() throws Exception {
resetStream();
assertEquals(0, ToolRunner.run(dfsAdmin, new String[] {"-report"}));
verifyNodesAndCorruptBlocks(numDn, numDn - 1, 1, 1, client, 0L, 0L);
+
+ // verify report command for list all DN types
+ resetStream();
+ String[] reportWithArg = new String[DFSAdmin.DFS_REPORT_ARGS.length + 1];
+ reportWithArg[0] = "-report";
+ int k=1;
+ for (int i = 0; i < DFSAdmin.DFS_REPORT_ARGS.length; i++) {
+ reportWithArg[k++] = DFSAdmin.DFS_REPORT_ARGS[i];
+ }
+ assertEquals(0, ToolRunner.run(dfsAdmin, reportWithArg));
}
}
once again, thanks for the PR.
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
Thanks @ayushtkn @virajjasani help me review this PR. |
Contributor
Author
yeah, I will review the code and if I find that the other commands have the same problem, I will fix them as well. |
Contributor
Author
Thanks for the suggestion, I think this change will solve the problem better. |
Contributor
Author
|
Update PR. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Fantastic, thank you @haiyang1987! |
virajjasani
approved these changes
May 20, 2023
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1 (non-binding) for the changes overall.
For method line to be within 150 as per checkstyle warning, you can also create a small private method, move the logic there and call it from main test method.
Member
|
we can live with that 150 checkstlye warning, some tests are indeed huge, didn't like this checkstyle warning itself, should relax this a bit for atleast for test code. |
asfgit
pushed a commit
that referenced
this pull request
May 21, 2023
… in DFSAdmin (#5667). Contributed by Haiyang Hu. Reviewed-by: Viraj Jasani <[email protected]> Signed-off-by: Ayush Saxena <[email protected]>
Contributor
Author
noted. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: HDFS-17017
Currently, the DFSAdmin report command should support a maximum number of arguments of 7， such as :
hdfs dfsadmin [-report] [-live] [-dead] [-decommissioning] [-enteringmaintenance] [-inmaintenance] [-slownodes]