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 9 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 @@ -86,9 +86,22 @@ public void handle() {
exitWithError("Error initializing client or table name: " + exp.getMessage());
}

if (client == null) {
exitWithError("Client initialization failed.");
return;
}
if (tableName == null) {
exitWithError("Table name could not be determined.");
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());
}
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 @@ -49,10 +49,14 @@ 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);

if (gCatalog != null) {
printProperties(gCatalog.properties());
}

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?

exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
Expand Down
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,41 @@ 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) {
exitWithError("No columns found for the specified table.");
return;
}

StringBuilder all = new StringBuilder();
all.append("name,datatype,comment,nullable,auto_increment").append(System.lineSeparator());

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

all.append(column.name()).append(",");
all.append(column.dataType() != null ? column.dataType().simpleString() : "UNKNOWN")
.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.

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

printResults(all.toString());

} 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,10 +59,15 @@ 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());
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException err) {
Expand All @@ -73,7 +78,12 @@ public void handle() {
exitWithError(exp.getMessage());
}

Map<String, String> properties = gFileset.properties();
printProperties(properties);
// Null check for gFileset before accessing its properties
if (gFileset == null) {
exitWithError("Failed to load fileset: " + fileset);
return;
}

printProperties(gFileset.properties() != null ? gFileset.properties() : Collections.emptyMap());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,21 @@ 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);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (Exception exp) {
exitWithError(exp.getMessage());
}

Map<String, String> properties = gMetalake.properties();
if (gMetalake == null) {
exitWithError("Metalake not found: " + metalake);
return;
}

Map<String, String> properties = gMetalake.properties();
printProperties(properties);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,10 @@ public ListSchemaProperties(
@Override
public void handle() {
Schema gSchema = null;
GravitinoClient client = null;

try {
GravitinoClient client = buildClient(metalake);
client = buildClient(metalake);
gSchema = client.loadCatalog(catalog).asSchemas().loadSchema(schema);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
Expand All @@ -68,6 +70,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,10 +59,12 @@ public ListTableProperties(
/** List the properties of a table. */
@Override
public void handle() {
GravitinoClient client = null;
Table gTable = null;

try {
client = buildClient(metalake);
NameIdentifier name = NameIdentifier.of(schema, table);
GravitinoClient client = buildClient(metalake);
gTable = client.loadCatalog(catalog).asTableCatalog().loadTable(name);
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
Expand All @@ -76,6 +78,11 @@ public void handle() {
exitWithError(exp.getMessage());
}

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

Map<String, String> properties = gTable.properties();
printProperties(properties);
}
Expand Down
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
}
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,18 @@ public void handle() {
try {
GravitinoClient client = buildClient(metalake);
gTag = client.getTag(tag);

} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchTagException err) {
exitWithError(ErrorMessages.UNKNOWN_TAG);
} catch (Exception exp) {
exitWithError(exp.getMessage());
exitWithError("An unexpected error occurred: " + exp.getMessage());
}

if (gTag == null) {
exitWithError(ErrorMessages.UNKNOWN_TAG);
return;
}

Map<String, String> properties = gTag.properties();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public void handle() {
exitWithError(exp.getMessage());
}

if (gTopic == null) {
exitWithError(ErrorMessages.UNKNOWN_TOPIC);
return;
}

Map<String, String> properties = gTopic.properties();
printProperties(properties);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,20 +68,28 @@ public void handle() {
ModelCatalog modelCatalog = client.loadCatalog(catalog).asModelCatalog();
gModel = modelCatalog.getModel(name);
versions = modelCatalog.listModelVersions(name);
} catch (NoSuchMetalakeException noSuchMetalakeException) {

} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchCatalogException noSuchCatalogException) {
} catch (NoSuchCatalogException err) {
exitWithError(ErrorMessages.UNKNOWN_CATALOG);
} catch (NoSuchSchemaException noSuchSchemaException) {
} catch (NoSuchSchemaException err) {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
} catch (NoSuchModelException noSuchModelException) {
} catch (NoSuchModelException err) {
exitWithError(ErrorMessages.UNKNOWN_MODEL);
} catch (Exception err) {
exitWithError(err.getMessage());
exitWithError("An unexpected error occurred: " + err.getMessage());
}

if (gModel == null) {
exitWithError(ErrorMessages.UNKNOWN_MODEL);
return;
}

String basicInfo =
String.format("Model name %s, latest version: %s%n", gModel.name(), gModel.latestVersion());
String versionInfo = Arrays.toString(versions);
String.format(
"Model Name: %s, Latest Version: %s%n", gModel.name(), gModel.latestVersion());
String versionInfo = "Versions: " + Arrays.toString(versions);
printResults(basicInfo + "versions: " + versionInfo);
}
}
Loading