adoroszlai commented on code in PR #7864:
URL: https://github.com/apache/ozone/pull/7864#discussion_r1954162888
##########
hadoop-ozone/dist/src/main/smoketest/basic/ozone-shell-lib.robot:
##########
@@ -188,6 +186,8 @@ Test key handling
Should Contain ${result} NOTICE.txt.1 exists
${result} = Execute ozone sh key get --force
${protocol}${server}/${volume}/bb1/key1 /tmp/NOTICE.txt.1
Should Not Contain ${result} NOTICE.txt.1 exists
+ ${result} = Execute and checkrc ozone sh key put
${protocol}${server}/${volume}/bb1/key1 sample.txt 255
+ Should Match Regexp ${result} Error: File not
found: .*
Review Comment:
I guess we can make this assertion a bit more specific by changing to
`Should Contain` and including `sample.txt` in the expected message.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -85,12 +89,17 @@ public int execute(String[] argv) {
}
protected void printError(Throwable error) {
- //message could be null in case of NPE. This is unexpected so we can
- //print out the stack trace.
- if (verbose || Strings.isNullOrEmpty(error.getMessage())) {
- error.printStackTrace(cmd.getErr());
+ if (error instanceof FileSystemException) {
+ String errorMessage = handleFileSystemException((FileSystemException)
error);
+ cmd.getErr().println(errorMessage);
Review Comment:
Let's keep the original `if-else`, and move handling of
`FileSystemException` to the `else` block. In other words, `verbose` mode
should still print the stack trace for `FileSystemException`.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -123,4 +132,25 @@ protected PrintWriter out() {
protected PrintWriter err() {
return cmd.getErr();
}
+
+ private String handleFileSystemException(FileSystemException e) {
+ // If reason is set, return the exception's message as it is.
+ if (e.getReason() != null) {
+ return e.getMessage();
+ }
+
+ // Otherwise, construct a custom message based on the type of exception
+ String errorMessage;
+ if (e instanceof NoSuchFileException) {
+ errorMessage = String.format("Error: File not found: %s", e.getFile());
+ } else if (e instanceof AccessDeniedException) {
+ errorMessage = String.format("Error: Access denied to file: %s",
e.getFile());
+ } else if (e instanceof FileAlreadyExistsException) {
+ errorMessage = String.format("Error: File already exists: %s",
e.getFile());
Review Comment:
Please use `getMessage()` instead of `getFile()`, to include `otherFile` in
the message. This is useful if the error affects that other file, e.g. when
trying to copy without "overwrite" flag.
Since the custom message is always prepended, use `errorMessage = "..." +
e.getMessage()` instead of `String.format()`.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -123,4 +132,25 @@ protected PrintWriter out() {
protected PrintWriter err() {
return cmd.getErr();
}
+
+ private String handleFileSystemException(FileSystemException e) {
+ // If reason is set, return the exception's message as it is.
+ if (e.getReason() != null) {
+ return e.getMessage();
+ }
+
+ // Otherwise, construct a custom message based on the type of exception
+ String errorMessage;
+ if (e instanceof NoSuchFileException) {
+ errorMessage = String.format("Error: File not found: %s", e.getFile());
+ } else if (e instanceof AccessDeniedException) {
+ errorMessage = String.format("Error: Access denied to file: %s",
e.getFile());
+ } else if (e instanceof FileAlreadyExistsException) {
+ errorMessage = String.format("Error: File already exists: %s",
e.getFile());
+ } else {
+ errorMessage = String.format("Error with file: %s. Details: %s",
e.getFile(), e.getMessage());
Review Comment:
Include exception type in the message, but can omit file, since it's
supposed to be included in `getMessage()`.
--
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]
