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

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
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ public void handle() {
exitWithError(exp.getMessage());
}

displayAuditInfo(result.auditInfo());
if (result != null) {
displayAuditInfo(result.auditInfo());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,27 +82,47 @@ public void handle() {
client = buildClient(metalake);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
return; // Stop execution
} catch (Exception exp) {
exitWithError("Error initializing client or table name: " + exp.getMessage());
return;
}

if (client == null) {
exitWithError("Client initialization failed.");
return;
}

try {
tableData = readTableCSV.parse(columnFile);
columns = readTableCSV.columns(tableData);
if (columns == null || columns.length == 0) {
exitWithError("No valid columns found in the provided file.");
return;
}
} catch (Exception exp) {
exitWithError("Error reading or parsing column file: " + exp.getMessage());
return;
}

try {
if (tableName == null) {
exitWithError("Table name could not be determined.");
return;
}
client.loadCatalog(catalog).asTableCatalog().createTable(tableName, columns, comment, null);
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
return;
} catch (NoSuchSchemaException err) {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
return;
} catch (TableAlreadyExistsException err) {
exitWithError(ErrorMessages.TABLE_EXISTS);
return;
} catch (Exception exp) {
exitWithError(exp.getMessage());
return;
}

printInformation(table + " created");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class FilesetDetails extends Command {
* @param context The command context.
* @param metalake The name of the metalake.
* @param catalog The name of the catalog.
* @param schema The name of the schenma.
* @param schema The name of the schema.
* @param fileset The name of the fileset.
*/
public FilesetDetails(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

package org.apache.gravitino.cli.commands;

import java.util.Map;
import org.apache.gravitino.Catalog;
import org.apache.gravitino.cli.CommandContext;
import org.apache.gravitino.cli.ErrorMessages;
Expand Down Expand Up @@ -49,19 +48,19 @@ public ListCatalogProperties(CommandContext context, String metalake, String cat
/** List the properties of a catalog. */
@Override
public void handle() {
Catalog gCatalog = null;
GravitinoClient client = buildClient(metalake);
try {
GravitinoClient client = buildClient(metalake);
gCatalog = client.loadCatalog(catalog);
Catalog gCatalog = client.loadCatalog(catalog);

if (gCatalog != null) {
printProperties(gCatalog.properties());
}
} 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?

exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}

Map<String, String> properties = gCatalog.properties();
printProperties(properties);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
* specific language governing permissions and limitations
* under the License.
*/

package org.apache.gravitino.cli.commands;

import com.google.common.base.Joiner;
Expand Down Expand Up @@ -51,18 +50,42 @@ public ListColumns(
/** Displays the details of a table's columns. */
@Override
public void handle() {
Column[] columns = null;

try {
NameIdentifier name = NameIdentifier.of(schema, table);
columns = tableCatalog().loadTable(name).columns();
Column[] columns = tableCatalog().loadTable(name).columns();

if (columns != null && columns.length > 0) {
StringBuilder all = new StringBuilder();
all.append("name,datatype,comment,nullable,auto_increment").append(System.lineSeparator());

for (Column column : columns) {
if (column == null) {
continue;
}

StringBuilder columnDetails = new StringBuilder();
columnDetails.append(column.name()).append(",");
columnDetails
.append(column.dataType() != null ? column.dataType().simpleString() : "UNKNOWN")
.append(",");
columnDetails.append(column.comment() != null ? column.comment() : "N/A").append(",");
columnDetails.append(column.nullable() ? "true" : "false").append(",");
columnDetails.append(column.autoIncrement() ? "true" : "false");

all.append(columnDetails).append(System.lineSeparator());
}

printResults(all.toString());
} else {
exitWithError("No columns found for the specified table.");
}
} catch (NoSuchTableException noSuchTableException) {
exitWithError(
ErrorMessages.UNKNOWN_TABLE + Joiner.on(".").join(metalake, catalog, schema, table));
ErrorMessages.UNKNOWN_TABLE
+ " "
+ Joiner.on(".").join(metalake, catalog, schema, table));
} catch (Exception exp) {
exitWithError(exp.getMessage());
exitWithError("An error occurred while retrieving column details: " + exp.getMessage());
}

printResults(columns);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

package org.apache.gravitino.cli.commands;

import java.util.Map;
import java.util.Collections;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.cli.CommandContext;
import org.apache.gravitino.cli.ErrorMessages;
Expand Down Expand Up @@ -59,21 +59,40 @@ public ListFilesetProperties(
@Override
public void handle() {
Fileset gFileset = null;
GravitinoClient client = null;

try {
NameIdentifier name = NameIdentifier.of(schema, fileset);
GravitinoClient client = buildClient(metalake);
client = buildClient(metalake);

gFileset = client.loadCatalog(catalog).asFilesetCatalog().loadFileset(name);
} catch (IllegalArgumentException exp) {
exitWithError("Invalid schema or fileset name: " + exp.getMessage());
return;
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
return;
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
return;
} catch (NoSuchSchemaException err) {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
return;
} catch (Exception exp) {
exitWithError(exp.getMessage());
return;
} finally {
if (client != null) {
client.close();
}
}

// Null check for gFileset before accessing its properties
if (gFileset == null) {
exitWithError("Failed to load fileset: " + fileset);
return;
}

Map<String, String> properties = gFileset.properties();
printProperties(properties);
printProperties(gFileset.properties() != null ? gFileset.properties() : Collections.emptyMap());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,22 @@ public ListMetalakeProperties(CommandContext context, String metalake) {
@Override
public void handle() {
Metalake gMetalake = null;
try {
GravitinoAdminClient client = buildAdminClient();

try (GravitinoAdminClient client = buildAdminClient()) { // Ensure resource cleanup
Copy link
Member

Choose a reason for hiding this comment

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

This is likely to close the client too early.

gMetalake = client.loadMetalake(metalake);

if (gMetalake == null) {
exitWithError("Metalake not found: " + metalake);
return;
}

} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}

Map<String, String> properties = gMetalake.properties();

printProperties(properties);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ public ListSchemaProperties(
@Override
public void handle() {
Schema gSchema = null;
try {
GravitinoClient client = buildClient(metalake);

try (GravitinoClient client = buildClient(metalake)) { // Ensure resource cleanup
gSchema = client.loadCatalog(catalog).asSchemas().loadSchema(schema);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
Expand All @@ -68,6 +68,11 @@ public void handle() {
exitWithError(exp.getMessage());
}

if (gSchema == null) {
exitWithError("Schema not found: " + schema);
return;
}

Map<String, String> properties = gSchema.properties();
printProperties(properties);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,17 @@ public ListTableProperties(
/** List the properties of a table. */
@Override
public void handle() {
Table gTable = null;
try {
try (GravitinoClient client = buildClient(metalake)) { // Ensures resource cleanup
NameIdentifier name = NameIdentifier.of(schema, table);
GravitinoClient client = buildClient(metalake);
gTable = client.loadCatalog(catalog).asTableCatalog().loadTable(name);
Table gTable = client.loadCatalog(catalog).asTableCatalog().loadTable(name);

if (gTable == null) {
exitWithError("Table not found: " + table);
return;
}

Map<String, String> properties = gTable.properties();
printProperties(properties);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
Expand All @@ -75,8 +81,5 @@ public void handle() {
} catch (Exception exp) {
exitWithError(exp.getMessage());
}

Map<String, String> properties = gTable.properties();
printProperties(properties);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public class ListTables extends TableCommand {
* @param context The command context.
* @param metalake The name of the metalake.
* @param catalog The name of the catalog.
* @param schema The name of the schenma.
* @param schema The name of the schema.
*/
public ListTables(CommandContext context, String metalake, String catalog, String schema) {
super(context, metalake, catalog);
Expand All @@ -47,43 +47,49 @@ public ListTables(CommandContext context, String metalake, String catalog, Strin
/** List the names of all tables in a schema. */
@Override
public void handle() {
NameIdentifier[] tables = null;
Namespace name = Namespace.of(schema);

try {
tables = tableCatalog().listTables(name);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}
Namespace name = Namespace.of(schema);
NameIdentifier[] tables = tableCatalog().listTables(name);

if (tables.length == 0) {
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

printInformation("No tables exist.");
return;
}

Table[] gTables = new Table[tables.length];
for (int i = 0; i < tables.length; i++) {
String tableName = tables[i].name();
gTables[i] =
new Table() {
Table[] gTables = new Table[tables.length];
for (int i = 0; i < tables.length; i++) {
String tableName = tables[i].name();
gTables[i] = createTableStub(tableName);
}

@Override
public String name() {
return tableName;
}
printResults(gTables);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}
}

@Override
public Column[] columns() {
return new Column[0];
}
/**
* Creates a stub Table instance with only the table name.
*
* @param tableName The name of the table.
* @return A minimal Table instance.
*/
private Table createTableStub(String tableName) {
return new Table() {
@Override
public String name() {
return tableName;
}

@Override
public Audit auditInfo() {
return null;
}
};
}
@Override
public Column[] columns() {
return new Column[0]; // Empty columns since only table names are needed
}

printResults(gTables);
@Override
public Audit auditInfo() {
return null; // No audit info needed for listing tables
}
};
}
}
Loading