Skip to content
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.

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

HDDS-64. OzoneClientException should extend IOException. #7403

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

nandakumar131
Copy link
Contributor

What changes were proposed in this pull request?

OzoneClientException should extend IOException.

To simplify exception handling OzoneClientException should extend IOException.

What is the link to the Apache JIRA

HDDS-64

How was this patch tested?

Existing unit test.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @nandakumar131 for taking up this super old issue.

@@ -96,7 +95,7 @@ public class FindMissingPadding extends Handler implements SubcommandWithParent
private final Set<OzoneKey> affectedKeys = new HashSet<>();

@Override
protected OzoneAddress getAddress() throws OzoneClientException {
protected OzoneAddress getAddress() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more specific exception should be kept in throws for all methods that do not otherwise throw IOException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I will update the patch.

Comment on lines 96 to 97
} catch (IOException ex) {
System.out.printf("Error: %s", ex.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

We can even completely remove catch.

Exception is handled by parent command (all other subcommands except GetServiceRolesSubcommand and UpdateRangerSubcommand rely on that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, let me fix both GetServiceRolesSubcommand and UpdateRangerSubcommand.

@nandakumar131
Copy link
Contributor Author

Thanks @adoroszlai for the review! Addressed all the review comments.

adoroszlai
adoroszlai previously approved these changes Nov 11, 2024
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @nandakumar131 for updating the patch.

@adoroszlai
Copy link
Contributor

Looks like a test relies on ozone admin om roles exiting with success even in case of failure.

Check om admin command                                                | FAIL |
255 != 0

Check om admin command
${result} = Execute and checkrc ozone admin om roles -id=omServiceIdDefault 0
Should Contain ${result} This command works only on OzoneManager HA cluster.

Suggested fix:

--- hadoop-ozone/dist/src/main/smoketest/om-ratis/testOMAdminCmd.robot
+++ hadoop-ozone/dist/src/main/smoketest/om-ratis/testOMAdminCmd.robot
@@ -22,5 +22,5 @@ Test Timeout        5 minutes
 *** Test Cases ***
 
 Check om admin command
-    ${result} =        Execute and checkrc              ozone admin om roles -id=omServiceIdDefault  0
+    ${result} =        Execute and ignore error         ozone admin om roles -id=omServiceIdDefault
                        Should Contain                   ${result}  This command works only on OzoneManager HA cluster.

@adoroszlai adoroszlai dismissed their stale review November 12, 2024 07:32

need to fix failing test

@nandakumar131
Copy link
Contributor Author

Thanks @adoroszlai for taking a look at the failure. I will update the PR with the suggested fix.

@adoroszlai adoroszlai merged commit 47c2409 into apache:master Nov 12, 2024
40 checks passed
@nandakumar131 nandakumar131 deleted the HDDS-64 branch November 13, 2024 05:50
@nandakumar131
Copy link
Contributor Author

Thanks @adoroszlai for the help and merging this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants