Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
shaofengshi merged PR #5793: URL: https://github.com/apache/gravitino/pull/5793 -- 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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
shaofengshi commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1882285805
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -162,39 +162,59 @@ private void handleMetalakeCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.DETAILS.equals(command)) {
- if (line.hasOption(GravitinoOptions.AUDIT)) {
-newMetalakeAudit(url, ignore, metalake).handle();
- } else {
-newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
- }
-} else if (CommandActions.LIST.equals(command)) {
- newListMetalakes(url, ignore, outputFormat).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateMetalake(url, ignore, metalake, comment).handle();
-} else if (CommandActions.DELETE.equals(command)) {
- boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteMetalake(url, ignore, force, metalake).handle();
-} else if (CommandActions.SET.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetMetalakeProperty(url, ignore, metalake, property, value).handle();
-} else if (CommandActions.REMOVE.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
-} else if (CommandActions.PROPERTIES.equals(command)) {
- newListMetalakeProperties(url, ignore, metalake).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
+switch (command) {
+ case CommandActions.DETAILS:
+if (line.hasOption(GravitinoOptions.AUDIT)) {
+ newMetalakeAudit(url, ignore, metalake).handle();
+} else {
+ newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
+}
+break;
+
+ case CommandActions.LIST:
+newListMetalakes(url, ignore, outputFormat).handle();
+break;
+
+ case CommandActions.CREATE:
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
+newCreateMetalake(url, ignore, metalake, comment).handle();
+break;
+
+ case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
-newUpdateMetalakeName(url, ignore, force, metalake, newName).handle();
- }
+newDeleteMetalake(url, ignore, force, metalake).handle();
+break;
+
+ case CommandActions.SET:
+String property = line.getOptionValue(GravitinoOptions.PROPERTY);
+String value = line.getOptionValue(GravitinoOptions.VALUE);
+newSetMetalakeProperty(url, ignore, metalake, property,
value).handle();
+break;
+
+ case CommandActions.REMOVE:
+property = line.getOptionValue(GravitinoOptions.PROPERTY);
+newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
+break;
+
+ case CommandActions.PROPERTIES:
+newListMetalakeProperties(url, ignore, metalake).handle();
+break;
+
+ case CommandActions.UPDATE:
+if (line.hasOption(GravitinoOptions.COMMENT)) {
+ comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
+}
+if (line.hasOption(GravitinoOptions.RENAME)) {
+ String newName = line.getOptionValue(GravitinoOptions.RENAME);
+ force = line.hasOption(GravitinoOptions.FORCE);
+ newUpdateMetalakeName(url, ignore, force, metalake,
newName).handle();
+}
+break;
+
+ default:
+System.err.println(ErrorMessages.UNSUPPORTED_COMMAND);
+break;
Review Comment:
All make sense, both are okay for me; Thanks a lot for Qiming's comment!
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1881335451
##
clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:
##
@@ -858,6 +868,43 @@ public void accept(ErrorResponse errorResponse) {
}
}
+ /** Error handler specific to Credential operations. */
+ @SuppressWarnings("FormatStringAnnotation")
+ private static class CredentialErrorHandler extends RestErrorHandler {
+
+private static final CredentialErrorHandler INSTANCE = new
CredentialErrorHandler();
+
+@Override
+public void accept(ErrorResponse errorResponse) {
+ String errorMessage = formatErrorMessage(errorResponse);
+
+ switch (errorResponse.getCode()) {
+case ErrorConstants.ILLEGAL_ARGUMENTS_CODE:
+ throw new IllegalArgumentException(errorMessage);
+
+case ErrorConstants.NOT_FOUND_CODE:
+ if
(errorResponse.getType().equals(NoSuchMetalakeException.class.getSimpleName()))
{
+throw new NoSuchMetalakeException(errorMessage);
+ } else if (errorResponse
Review Comment:
This is out of scope of this PR as it's outside the CLI code or any changes
made by this PR. Best to raise a new issue for it.
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1881335330
##
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java:
##
@@ -98,6 +101,23 @@ Catalog createCatalog(
Map properties)
throws NoSuchMetalakeException, CatalogAlreadyExistsException;
+ /**
+ * Create a managed catalog with specified catalog name, type, comment, and
properties.
+ *
+ * @param catalogName the name of the catalog.
+ * @param type the type of the catalog.
+ * @param comment the comment of the catalog.
+ * @param properties the properties of the catalog.
+ * @return The created catalog.
+ * @throws NoSuchMetalakeException If the metalake does not exist.
+ * @throws CatalogAlreadyExistsException If the catalog already exists.
+ */
+ default Catalog createCatalog(
+ String catalogName, Catalog.Type type, String comment, Map properties)
+ throws NoSuchMetalakeException, CatalogAlreadyExistsException {
+return createCatalog(catalogName, type, null, comment, properties);
Review Comment:
This is out of scope of this PR as it's outside the CLI code or any changes
made by this PR. Best to raise a new issue for it.
##
api/src/main/java/org/apache/gravitino/credential/GCSTokenCredential.java:
##
@@ -70,4 +83,11 @@ public Map credentialInfo() {
public String token() {
return token;
}
+
+ private void validate(String token, long expireTimeInMs) {
Review Comment:
This is out of scope of this PR as it's outside the CLI code or any changes
made by this PR. Best to raise a new issue for it.
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
tengqm commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1879129401
##
api/src/main/java/org/apache/gravitino/SupportsCatalogs.java:
##
@@ -98,6 +101,23 @@ Catalog createCatalog(
Map properties)
throws NoSuchMetalakeException, CatalogAlreadyExistsException;
+ /**
+ * Create a managed catalog with specified catalog name, type, comment, and
properties.
+ *
+ * @param catalogName the name of the catalog.
+ * @param type the type of the catalog.
+ * @param comment the comment of the catalog.
+ * @param properties the properties of the catalog.
+ * @return The created catalog.
+ * @throws NoSuchMetalakeException If the metalake does not exist.
+ * @throws CatalogAlreadyExistsException If the catalog already exists.
+ */
+ default Catalog createCatalog(
+ String catalogName, Catalog.Type type, String comment, Map properties)
+ throws NoSuchMetalakeException, CatalogAlreadyExistsException {
+return createCatalog(catalogName, type, null, comment, properties);
Review Comment:
```suggestion
String name, Catalog.Type type, String comment, Map
properties)
throws NoSuchMetalakeException, CatalogAlreadyExistsException {
return createCatalog(name, type, null, comment, properties);
```
##
api/src/main/java/org/apache/gravitino/credential/GCSTokenCredential.java:
##
@@ -70,4 +83,11 @@ public Map credentialInfo() {
public String token() {
return token;
}
+
+ private void validate(String token, long expireTimeInMs) {
Review Comment:
How about embed this method into `initialize`?
The GCSTokenCredential is immutable IIUC, so I suppose that this validation
would be invoked only from `initialize`.
We can move this out when this assumption doesn't hold in the future.
##
clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:
##
@@ -858,6 +868,43 @@ public void accept(ErrorResponse errorResponse) {
}
}
+ /** Error handler specific to Credential operations. */
+ @SuppressWarnings("FormatStringAnnotation")
+ private static class CredentialErrorHandler extends RestErrorHandler {
+
+private static final CredentialErrorHandler INSTANCE = new
CredentialErrorHandler();
+
+@Override
+public void accept(ErrorResponse errorResponse) {
+ String errorMessage = formatErrorMessage(errorResponse);
+
+ switch (errorResponse.getCode()) {
+case ErrorConstants.ILLEGAL_ARGUMENTS_CODE:
+ throw new IllegalArgumentException(errorMessage);
+
+case ErrorConstants.NOT_FOUND_CODE:
+ if
(errorResponse.getType().equals(NoSuchMetalakeException.class.getSimpleName()))
{
+throw new NoSuchMetalakeException(errorMessage);
+ } else if (errorResponse
Review Comment:
Em ... I don't think we want to make this exception an explicit one.
The reason is that we may intentionally make this less clear, for security's
sake.
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
tengqm commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875865503
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -162,39 +162,59 @@ private void handleMetalakeCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.DETAILS.equals(command)) {
- if (line.hasOption(GravitinoOptions.AUDIT)) {
-newMetalakeAudit(url, ignore, metalake).handle();
- } else {
-newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
- }
-} else if (CommandActions.LIST.equals(command)) {
- newListMetalakes(url, ignore, outputFormat).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateMetalake(url, ignore, metalake, comment).handle();
-} else if (CommandActions.DELETE.equals(command)) {
- boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteMetalake(url, ignore, force, metalake).handle();
-} else if (CommandActions.SET.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetMetalakeProperty(url, ignore, metalake, property, value).handle();
-} else if (CommandActions.REMOVE.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
-} else if (CommandActions.PROPERTIES.equals(command)) {
- newListMetalakeProperties(url, ignore, metalake).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
+switch (command) {
+ case CommandActions.DETAILS:
+if (line.hasOption(GravitinoOptions.AUDIT)) {
+ newMetalakeAudit(url, ignore, metalake).handle();
+} else {
+ newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
+}
+break;
+
+ case CommandActions.LIST:
+newListMetalakes(url, ignore, outputFormat).handle();
+break;
+
+ case CommandActions.CREATE:
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
+newCreateMetalake(url, ignore, metalake, comment).handle();
+break;
+
+ case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
-newUpdateMetalakeName(url, ignore, force, metalake, newName).handle();
- }
+newDeleteMetalake(url, ignore, force, metalake).handle();
+break;
+
+ case CommandActions.SET:
+String property = line.getOptionValue(GravitinoOptions.PROPERTY);
+String value = line.getOptionValue(GravitinoOptions.VALUE);
+newSetMetalakeProperty(url, ignore, metalake, property,
value).handle();
+break;
+
+ case CommandActions.REMOVE:
+property = line.getOptionValue(GravitinoOptions.PROPERTY);
+newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
+break;
+
+ case CommandActions.PROPERTIES:
+newListMetalakeProperties(url, ignore, metalake).handle();
+break;
+
+ case CommandActions.UPDATE:
+if (line.hasOption(GravitinoOptions.COMMENT)) {
+ comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
+}
+if (line.hasOption(GravitinoOptions.RENAME)) {
+ String newName = line.getOptionValue(GravitinoOptions.RENAME);
+ force = line.hasOption(GravitinoOptions.FORCE);
+ newUpdateMetalakeName(url, ignore, force, metalake,
newName).handle();
+}
+break;
+
+ default:
+System.err.println(ErrorMessages.UNSUPPORTED_COMMAND);
+break;
Review Comment:
makes sense. When we have a list of things to maintain, I am more inclined
to have
it sorted, in an order that is easy to be understood with a quick glimpse.
The order can be a shared view of the things or a similar common
understanding.
The alpha-beta order is only a fall back when people don't share the same
opinion
about why X comes after Y.
An sorted list is easy to review and easy to maintain. Say when you want to
add a
new case, people won't ask you why you place your new code here rather than
there.
Say when you want to find the logic for a specific case, you can quickly
locate
the code.
Anyway, so long we have a common understanding on a fixed order, this is
fine.
I'm not strongly opinionated wrt the way it is sorted.
--
This is an automated message from the Apach
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875325356
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -162,39 +162,59 @@ private void handleMetalakeCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.DETAILS.equals(command)) {
- if (line.hasOption(GravitinoOptions.AUDIT)) {
-newMetalakeAudit(url, ignore, metalake).handle();
- } else {
-newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
- }
-} else if (CommandActions.LIST.equals(command)) {
- newListMetalakes(url, ignore, outputFormat).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateMetalake(url, ignore, metalake, comment).handle();
-} else if (CommandActions.DELETE.equals(command)) {
- boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteMetalake(url, ignore, force, metalake).handle();
-} else if (CommandActions.SET.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetMetalakeProperty(url, ignore, metalake, property, value).handle();
-} else if (CommandActions.REMOVE.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
-} else if (CommandActions.PROPERTIES.equals(command)) {
- newListMetalakeProperties(url, ignore, metalake).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
+switch (command) {
+ case CommandActions.DETAILS:
+if (line.hasOption(GravitinoOptions.AUDIT)) {
+ newMetalakeAudit(url, ignore, metalake).handle();
+} else {
+ newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
+}
+break;
+
+ case CommandActions.LIST:
+newListMetalakes(url, ignore, outputFormat).handle();
+break;
+
+ case CommandActions.CREATE:
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
+newCreateMetalake(url, ignore, metalake, comment).handle();
+break;
+
+ case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
-newUpdateMetalakeName(url, ignore, force, metalake, newName).handle();
- }
+newDeleteMetalake(url, ignore, force, metalake).handle();
+break;
+
+ case CommandActions.SET:
+String property = line.getOptionValue(GravitinoOptions.PROPERTY);
+String value = line.getOptionValue(GravitinoOptions.VALUE);
+newSetMetalakeProperty(url, ignore, metalake, property,
value).handle();
+break;
+
+ case CommandActions.REMOVE:
+property = line.getOptionValue(GravitinoOptions.PROPERTY);
+newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
+break;
+
+ case CommandActions.PROPERTIES:
+newListMetalakeProperties(url, ignore, metalake).handle();
+break;
+
+ case CommandActions.UPDATE:
+if (line.hasOption(GravitinoOptions.COMMENT)) {
+ comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
+}
+if (line.hasOption(GravitinoOptions.RENAME)) {
+ String newName = line.getOptionValue(GravitinoOptions.RENAME);
+ force = line.hasOption(GravitinoOptions.FORCE);
+ newUpdateMetalakeName(url, ignore, force, metalake,
newName).handle();
+}
+break;
+
+ default:
+System.err.println(ErrorMessages.UNSUPPORTED_COMMAND);
+break;
Review Comment:
I don't think that helps anyone looking at the code and the current
consistent logical order works better as operation are in an expected order
e.g. remove after set and and delete after create, and the grouping of list and
details and grant and revoke.
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875329017
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -538,87 +679,93 @@ private void handleColumnCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.LIST.equals(command)) {
- newListColumns(url, ignore, metalake, catalog, schema, table).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- String position = line.getOptionValue(GravitinoOptions.POSITION);
- boolean nullable = true;
- boolean autoIncrement = false;
- String defaultValue = line.getOptionValue(GravitinoOptions.DEFAULT);
-
- if (line.hasOption(GravitinoOptions.NULL)) {
-nullable = line.getOptionValue(GravitinoOptions.NULL).equals("true");
- }
-
- if (line.hasOption(GravitinoOptions.AUTO)) {
-autoIncrement =
line.getOptionValue(GravitinoOptions.AUTO).equals("true");
- }
-
- newAddColumn(
- url,
- ignore,
- metalake,
- catalog,
- schema,
- table,
- column,
- datatype,
- comment,
- position,
- nullable,
- autoIncrement,
- defaultValue)
- .handle();
-} else if (CommandActions.DELETE.equals(command)) {
- newDeleteColumn(url, ignore, metalake, catalog, schema, table,
column).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
-String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateColumnComment(url, ignore, metalake, catalog, schema, table,
column, comment)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
-newUpdateColumnName(url, ignore, metalake, catalog, schema, table,
column, newName)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.DATATYPE)) {
-String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
-newUpdateColumnDatatype(url, ignore, metalake, catalog, schema, table,
column, datatype)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.POSITION)) {
-String position = line.getOptionValue(GravitinoOptions.POSITION);
-newUpdateColumnPosition(url, ignore, metalake, catalog, schema, table,
column, position)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.NULL)) {
-if (line.getOptionValue(GravitinoOptions.NULL).equals("true")) {
- newUpdateColumnNullability(url, ignore, metalake, catalog, schema,
table, column, true)
- .handle();
-} else if (line.getOptionValue(GravitinoOptions.NULL).equals("false"))
{
- newUpdateColumnNullability(url, ignore, metalake, catalog, schema,
table, column, false)
+switch (command) {
+ case CommandActions.LIST:
+newListColumns(url, ignore, metalake, catalog, schema, table).handle();
+break;
+
+ case CommandActions.CREATE:
+{
+ String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
+ String comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ String position = line.getOptionValue(GravitinoOptions.POSITION);
+ boolean nullable =
+ !line.hasOption(GravitinoOptions.NULL)
+ || line.getOptionValue(GravitinoOptions.NULL).equals("true");
+ boolean autoIncrement =
+ line.hasOption(GravitinoOptions.AUTO)
+ && line.getOptionValue(GravitinoOptions.AUTO).equals("true");
+ String defaultValue = line.getOptionValue(GravitinoOptions.DEFAULT);
+
+ newAddColumn(
+ url,
+ ignore,
+ metalake,
+ catalog,
+ schema,
+ table,
+ column,
+ datatype,
+ comment,
+ position,
+ nullable,
+ autoIncrement,
+ defaultValue)
.handle();
+ break;
}
- }
- if (line.hasOption(GravitinoOptions.AUTO)) {
-if (line.getOptionValue(GravitinoOptions.AUTO).equals("true")) {
- newUpdateColumnAutoIncrement(url, ignore, metalake, catalog, schema,
table, column, true)
- .handle();
-} else if (line.getOptionValue(GravitinoOptions.AUTO).equals("false"))
{
- newUpdateColumnAutoIncrement(url, ignore, metalake, catalog, schema,
table, column, false)
- .handle();
+
+ case
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875325356
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -162,39 +162,59 @@ private void handleMetalakeCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.DETAILS.equals(command)) {
- if (line.hasOption(GravitinoOptions.AUDIT)) {
-newMetalakeAudit(url, ignore, metalake).handle();
- } else {
-newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
- }
-} else if (CommandActions.LIST.equals(command)) {
- newListMetalakes(url, ignore, outputFormat).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateMetalake(url, ignore, metalake, comment).handle();
-} else if (CommandActions.DELETE.equals(command)) {
- boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteMetalake(url, ignore, force, metalake).handle();
-} else if (CommandActions.SET.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetMetalakeProperty(url, ignore, metalake, property, value).handle();
-} else if (CommandActions.REMOVE.equals(command)) {
- String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
-} else if (CommandActions.PROPERTIES.equals(command)) {
- newListMetalakeProperties(url, ignore, metalake).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
+switch (command) {
+ case CommandActions.DETAILS:
+if (line.hasOption(GravitinoOptions.AUDIT)) {
+ newMetalakeAudit(url, ignore, metalake).handle();
+} else {
+ newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
+}
+break;
+
+ case CommandActions.LIST:
+newListMetalakes(url, ignore, outputFormat).handle();
+break;
+
+ case CommandActions.CREATE:
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
+newCreateMetalake(url, ignore, metalake, comment).handle();
+break;
+
+ case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
-newUpdateMetalakeName(url, ignore, force, metalake, newName).handle();
- }
+newDeleteMetalake(url, ignore, force, metalake).handle();
+break;
+
+ case CommandActions.SET:
+String property = line.getOptionValue(GravitinoOptions.PROPERTY);
+String value = line.getOptionValue(GravitinoOptions.VALUE);
+newSetMetalakeProperty(url, ignore, metalake, property,
value).handle();
+break;
+
+ case CommandActions.REMOVE:
+property = line.getOptionValue(GravitinoOptions.PROPERTY);
+newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
+break;
+
+ case CommandActions.PROPERTIES:
+newListMetalakeProperties(url, ignore, metalake).handle();
+break;
+
+ case CommandActions.UPDATE:
+if (line.hasOption(GravitinoOptions.COMMENT)) {
+ comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
+}
+if (line.hasOption(GravitinoOptions.RENAME)) {
+ String newName = line.getOptionValue(GravitinoOptions.RENAME);
+ force = line.hasOption(GravitinoOptions.FORCE);
+ newUpdateMetalakeName(url, ignore, force, metalake,
newName).handle();
+}
+break;
+
+ default:
+System.err.println(ErrorMessages.UNSUPPORTED_COMMAND);
+break;
Review Comment:
I don't think that helps anyone looking at the code and the current
consistent logical order works better as operations are in an expected order
e.g. remove after set and and delete after create, and the grouping of list and
details and grant and revoke.
--
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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875322234
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -652,21 +799,29 @@ private void handleOwnerCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.DETAILS.equals(command)) {
- newOwnerDetails(url, ignore, metalake, entityName, entity).handle();
-} else if (CommandActions.SET.equals(command)) {
- String owner = line.getOptionValue(GravitinoOptions.USER);
- String group = line.getOptionValue(GravitinoOptions.GROUP);
-
- if (owner != null && group == null) {
-newSetOwner(url, ignore, metalake, entityName, entity, owner,
false).handle();
- } else if (owner == null && group != null) {
-newSetOwner(url, ignore, metalake, entityName, entity, group,
true).handle();
- } else {
-System.err.println(ErrorMessages.INVALID_SET_COMMAND);
- }
-} else {
- System.err.println(ErrorMessages.UNSUPPORTED_ACTION);
+switch (command) {
+ case CommandActions.DETAILS:
+newOwnerDetails(url, ignore, metalake, entityName, entity).handle();
+break;
+
+ case CommandActions.SET:
+{
+ String owner = line.getOptionValue(GravitinoOptions.USER);
+ String group = line.getOptionValue(GravitinoOptions.GROUP);
+
+ if (owner != null && group == null) {
+newSetOwner(url, ignore, metalake, entityName, entity, owner,
false).handle();
+ } else if (owner == null && group != null) {
+newSetOwner(url, ignore, metalake, entityName, entity, group,
true).handle();
+ } else {
+System.err.println(ErrorMessages.INVALID_SET_COMMAND);
+ }
+ break;
+}
Review Comment:
No they cannot - see 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]
Re: [PR] [#5685] Change main command section of Gravitino CLI to use switch statements [gravitino]
justinmclean commented on code in PR #5793:
URL: https://github.com/apache/gravitino/pull/5793#discussion_r1875321828
##
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##
@@ -538,87 +679,93 @@ private void handleColumnCommand() {
Command.setAuthenticationMode(auth, userName);
-if (CommandActions.LIST.equals(command)) {
- newListColumns(url, ignore, metalake, catalog, schema, table).handle();
-} else if (CommandActions.CREATE.equals(command)) {
- String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
- String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- String position = line.getOptionValue(GravitinoOptions.POSITION);
- boolean nullable = true;
- boolean autoIncrement = false;
- String defaultValue = line.getOptionValue(GravitinoOptions.DEFAULT);
-
- if (line.hasOption(GravitinoOptions.NULL)) {
-nullable = line.getOptionValue(GravitinoOptions.NULL).equals("true");
- }
-
- if (line.hasOption(GravitinoOptions.AUTO)) {
-autoIncrement =
line.getOptionValue(GravitinoOptions.AUTO).equals("true");
- }
-
- newAddColumn(
- url,
- ignore,
- metalake,
- catalog,
- schema,
- table,
- column,
- datatype,
- comment,
- position,
- nullable,
- autoIncrement,
- defaultValue)
- .handle();
-} else if (CommandActions.DELETE.equals(command)) {
- newDeleteColumn(url, ignore, metalake, catalog, schema, table,
column).handle();
-} else if (CommandActions.UPDATE.equals(command)) {
- if (line.hasOption(GravitinoOptions.COMMENT)) {
-String comment = line.getOptionValue(GravitinoOptions.COMMENT);
-newUpdateColumnComment(url, ignore, metalake, catalog, schema, table,
column, comment)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.RENAME)) {
-String newName = line.getOptionValue(GravitinoOptions.RENAME);
-newUpdateColumnName(url, ignore, metalake, catalog, schema, table,
column, newName)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.DATATYPE)) {
-String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
-newUpdateColumnDatatype(url, ignore, metalake, catalog, schema, table,
column, datatype)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.POSITION)) {
-String position = line.getOptionValue(GravitinoOptions.POSITION);
-newUpdateColumnPosition(url, ignore, metalake, catalog, schema, table,
column, position)
-.handle();
- }
- if (line.hasOption(GravitinoOptions.NULL)) {
-if (line.getOptionValue(GravitinoOptions.NULL).equals("true")) {
- newUpdateColumnNullability(url, ignore, metalake, catalog, schema,
table, column, true)
- .handle();
-} else if (line.getOptionValue(GravitinoOptions.NULL).equals("false"))
{
- newUpdateColumnNullability(url, ignore, metalake, catalog, schema,
table, column, false)
+switch (command) {
+ case CommandActions.LIST:
+newListColumns(url, ignore, metalake, catalog, schema, table).handle();
+break;
+
+ case CommandActions.CREATE:
+{
+ String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
+ String comment = line.getOptionValue(GravitinoOptions.COMMENT);
+ String position = line.getOptionValue(GravitinoOptions.POSITION);
+ boolean nullable =
+ !line.hasOption(GravitinoOptions.NULL)
+ || line.getOptionValue(GravitinoOptions.NULL).equals("true");
+ boolean autoIncrement =
+ line.hasOption(GravitinoOptions.AUTO)
+ && line.getOptionValue(GravitinoOptions.AUTO).equals("true");
+ String defaultValue = line.getOptionValue(GravitinoOptions.DEFAULT);
+
+ newAddColumn(
+ url,
+ ignore,
+ metalake,
+ catalog,
+ schema,
+ table,
+ column,
+ datatype,
+ comment,
+ position,
+ nullable,
+ autoIncrement,
+ defaultValue)
.handle();
+ break;
}
- }
- if (line.hasOption(GravitinoOptions.AUTO)) {
-if (line.getOptionValue(GravitinoOptions.AUTO).equals("true")) {
- newUpdateColumnAutoIncrement(url, ignore, metalake, catalog, schema,
table, column, true)
- .handle();
-} else if (line.getOptionValue(GravitinoOptions.AUTO).equals("false"))
{
- newUpdateColumnAutoIncrement(url, ignore, metalake, catalog, schema,
table, column, false)
- .handle();
+
+ case
