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.

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

Jump to bottom

refactor(cli): Improve CLI commands with better error handling and output formatting #6573

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

refactor(cli): Improve CLI commands with better error handling and output formatting #6573

wants to merge 13 commits into from

Conversation

Brijeshthummar02
Copy link
Contributor

What changes were proposed in this pull request?

This PR enhances multiple CLI commands by improving error handling, resource management, and output formatting. The following changes were made:

  • Improved exception handling for better debugging and user feedback.
  • Enhanced error messages for missing entities to provide clearer guidance.
  • Implemented try-with-resources for better resource management where applicable.
  • Ensured proper handling of empty responses and formatted outputs for better readability.
  • Standardized approach to handling CLI command execution errors.

Why are the changes needed?

These improvements ensure that CLI commands provide more reliable feedback to users, prevent unhandled exceptions, and improve resource management. The changes also enhance maintainability and debugging for developers.

Fix: #6528

Does this PR introduce any user-facing change?

Yes, the following user-facing changes are introduced:

  • Improved error messages for missing entities.
  • Better formatting for command outputs.
  • Safer resource handling to prevent unexpected failures.

@Brijeshthummar02
Copy link
Contributor Author

Brijeshthummar02 commented Feb 28, 2025 •
edited
Loading

@justinmclean it was taking too long for me as there where multiple files but finally made my way, now it's up to you, you may go through the changes and feel free to mention also added comments where i feel it should be there for better readability.

@Brijeshthummar02
Copy link
Contributor Author

@yuqi1129 need your review on this PR.

@yuqi1129
Copy link
Contributor

@yuqi1129 need your review on this PR.

Got

@Brijeshthummar02
Copy link
Contributor Author

@yuqi1129 That was too quick, Thanks!!

@Brijeshthummar02
Copy link
Contributor Author

@yuqi1129 need your review on this PR again

@Brijeshthummar02
Copy link
Contributor Author

@justinmclean @tengqm tried to fix and resolve as suggested can just go through it ones also removed what i feel is not needed.

@Brijeshthummar02
Copy link
Contributor Author

@justinmclean tried to fix all return statement, using it was my best practice but for this repo as u said we don't generally need so removed apart from that fixed all if statements outside the try block. Also thankyouuu! for giving reviews always as there are multiples files so this is taking way more time.

tengqm
tengqm reviewed Mar 2, 2025
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListCatalogProperties.java
try {
GravitinoClient client = buildClient(metalake);
gCatalog = client.loadCatalog(catalog);
Catalog gCatalog = client.loadCatalog(catalog);
} catch (NoSuchMetalakeException err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh No. This code won't work.
Checking gCatalog on line 54 before it is declared on line 59?
Throw NoSuchMetalakeException while metalake is used on line 52?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will loadCatalog throw NoSuchMetalakeException?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On checking loadCatalog it will generally not throw NoSuchMetalakeException, and hence removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The original code buildClient(metalake); may throw this exception, right?
You moved that line out of the try block for unknown reason.
Now you know why I'm concerned?

@Brijeshthummar02
Copy link
Contributor Author

@tengqm made relevant changes.

@Brijeshthummar02 Brijeshthummar02 requested a review from tengqm March 2, 2025 13:06
@tengqm
Copy link
Contributor

tengqm commented Mar 3, 2025

@tengqm made relevant changes.

Two points here:

  • If you don't agree to a feedback, you can leave it there, but don't mark it as "resolved". Marking a feedback as "resolved" without changing the code accordingly means "I don't care" or "I'm ignoring what you suggested.".
  • For the "return" statement, I would say that again. I don't have a strong opinion on whether we should add it. But if there is decision, we need to be consistent in the code.

@Brijeshthummar02
Copy link
Contributor Author

@tengqm Apologies for any confusion. As Justin mentioned, the decision was to remove the return statement, and I’ve tried to remove it from almost all files. I didn’t mean to take your feedback the wrong way—I truly appreciate the review. I might have missed checking all files since I’m currently in the middle of my university exams while still staying active in open-source contributions.

I’ll make sure to correct it properly. Would it be fine if I just remove the return statement from all files to ensure consistency?

@Brijeshthummar02
Copy link
Contributor Author

@tengqm Just to confirm, should I go ahead and remove the return statement from all cases, including if statements?

@tengqm
Copy link
Contributor

tengqm commented Mar 3, 2025

@tengqm Just to confirm, should I go ahead and remove the return statement from all cases, including if statements?

Yup. If the decision was that 'return' is unnecessary, please remove them all.

@Brijeshthummar02 Brijeshthummar02 force-pushed the feature/cli-command-improvements branch from 59fb7e3 to 42ca8c9 Compare March 3, 2025 12:16
tengqm
tengqm reviewed Mar 3, 2025
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListColumns.java
.append(",");
all.append(column.comment() != null ? column.comment() : "N/A").append(",");
all.append(column.nullable() ? "true" : "false").append(",");
all.append(column.autoIncrement() ? "true" : "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this column is okay and consistent with other format of outputs.

@Brijeshthummar02 Brijeshthummar02 force-pushed the feature/cli-command-improvements branch from f013623 to 349b343 Compare March 3, 2025 13:10
@Brijeshthummar02
Copy link
Contributor Author

@tengqm do we need any changes??

justinmclean
justinmclean reviewed Mar 6, 2025
View reviewed changes
clients/cli/src/main/java/org/apache/gravitino/cli/commands/ListTables.java
printInformation("No tables exist.");
return;
}
if (tables == null || tables.length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

this code should be outside the try block

@Brijeshthummar02 Brijeshthummar02 force-pushed the feature/cli-command-improvements branch from c7bb843 to 108c398 Compare March 7, 2025 12:05
@Brijeshthummar02
Copy link
Contributor Author

@jerryshao i really appreciate your review on this PR.

@justinmclean
Copy link
Member

@Brijeshthummar02, there are still some outstanding, unaddressed comments. You have also resolved several issues without fixing them. It's getting there, but I think it still needs a little work.

My main concern is that the added try (GravitinoClient client = buildClient(metalake)) will close the client too soon, our unit tests are unlikely to catch this. I'll take a look on Monday to see if this is an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmclean justinmclean justinmclean left review comments

@tengqm tengqm tengqm left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Fix potential null pointer expections in CLI
4 participants
@Brijeshthummar02 @yuqi1129 @justinmclean @tengqm