adoroszlai commented on code in PR #10160:
URL: https://github.com/apache/ozone/pull/10160#discussion_r3195766035
##########
hadoop-ozone/cli-admin/src/test/java/org/apache/hadoop/hdds/scm/cli/container/TestReconcileSubcommand.java:
##########
@@ -302,8 +302,8 @@ public void testReconcileHandlesValidAndInvalidContainers()
throws Exception {
});
// Should have error messages for EC containers
- assertThatOutput(errContent).contains("Failed to trigger reconciliation
for container 1: " + EC_CONTAINER_MESSAGE);
- assertThatOutput(errContent).contains("Failed to trigger reconciliation
for container 3: " + EC_CONTAINER_MESSAGE);
+ assertThatOutput(errContent).contains(EC_CONTAINER_MESSAGE);
+ assertThatOutput(errContent).contains(EC_CONTAINER_MESSAGE);
Review Comment:
The assertions are duplicate now.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java:
##########
@@ -128,8 +129,7 @@ private void executeReconcile(ScmClient scmClient) {
System.out.println("Reconciliation has been triggered for container "
+ containerID);
successCount++;
} catch (Exception ex) {
- System.err.println("Failed to trigger reconciliation for container " +
containerID + ": " +
- getExceptionMessage(ex));
+ System.err.println(getExceptionMessage(ex));
failureCount++;
Review Comment:
If it fails due to authentication, we should stop the loop, cannot expect
different results for the other containers. (Also for `container info`.)
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HAUtils.java:
##########
@@ -414,7 +414,12 @@ public RetryAction shouldRetry(Exception e, int retries,
int failovers, boolean
return retriableTask.call();
} catch (Exception ex) {
if (containsAccessControlException(ex)) {
- throw new AccessControlException();
+ Throwable cause = ex;
+ while (cause != null && !(cause instanceof AccessControlException)) {
+ cause = cause.getCause();
+ }
Review Comment:
`containsAccessControlException` already searches for
`AccessControlException`. Can we avoid duplicate loop?
##########
hadoop-hdds/cli-common/src/main/java/org/apache/hadoop/hdds/cli/GenericCli.java:
##########
@@ -88,18 +89,21 @@ public int execute(String[] argv) {
@Override
public void printError(Throwable error) {
- //message could be null in case of NPE. This is unexpected so we can
- //print out the stack trace.
final String rawMessage = error.getMessage();
if (verbose || rawMessage == null || rawMessage.isEmpty()) {
error.printStackTrace(cmd.getErr());
+ return;
+ }
+ String aclLine = HddsUtils.formatAccessControlExceptionLine(error);
+ if (aclLine != null) {
+ cmd.getErr().println(aclLine);
+ return;
+ }
+ if (error instanceof FileSystemException) {
+ String errorMessage = handleFileSystemException((FileSystemException)
error);
+ cmd.getErr().println(errorMessage);
} else {
Review Comment:
nit: let's replace `if-else` with `if-return` to be consistent with the
other cases above
--
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]
