tengqm commented on code in PR #5641:
URL: https://github.com/apache/gravitino/pull/5641#discussion_r1853151664
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -444,44 +449,66 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- new CreateTag(url, ignore, metalake, tag, comment).handle();
+ new CreateTag(url, ignore, metalake, tags, comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- new DeleteTag(url, ignore, force, metalake, tag).handle();
+ new DeleteTag(url, ignore, force, metalake, tags).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (name == null && property != null && value != null) {
- new SetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ if (tags.length > 1) {
+ System.err.println(ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return;
+ }
+ new SetTagProperty(url, ignore, metalake, tags[0], property,
value).handle();
} else if (name != null && property == null && value == null) {
- new TagEntity(url, ignore, metalake, name, tag).handle();
+ new TagEntity(url, ignore, metalake, name, tags).handle();
} else {
System.err.println(ErrorMessages.INVALID_SET_COMMAND);
}
} else if (CommandActions.REMOVE.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
if (property != null) {
- new RemoveTagProperty(url, ignore, metalake, tag, property).handle();
+ if (tags.length > 1) {
+ System.err.println(ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return;
+ }
+ new RemoveTagProperty(url, ignore, metalake, tags[0],
property).handle();
} else if (name != null) {
- new UntagEntity(url, ignore, metalake, name, tag).handle();
+ new UntagEntity(url, ignore, metalake, name, tags).handle();
} else {
System.err.println(ErrorMessages.INVALID_REMOVE_COMMAND);
}
} else if (CommandActions.PROPERTIES.equals(command)) {
- new ListTagProperties(url, ignore, metalake, tag).handle();
+ new ListTagProperties(url, ignore, metalake, tags[0]).handle();
} else if (CommandActions.UPDATE.equals(command)) {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- new UpdateTagComment(url, ignore, metalake, tag, comment).handle();
+ new UpdateTagComment(url, ignore, metalake, tags[0], comment).handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- new UpdateTagName(url, ignore, metalake, tag, newName).handle();
+ new UpdateTagName(url, ignore, metalake, tags[0], newName).handle();
}
}
}
+ private boolean validateMultipleTagOption(String[] tags) {
+ switch (command) {
+ case CommandActions.CREATE:
+ case CommandActions.DELETE:
+ case CommandActions.SET:
+ case CommandActions.REMOVE:
+ return true;
+ default:
+ if (tags.length > 1) {
+ return false;
+ }
+ return true;
Review Comment:
if tags is of length 0, this returns true.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -432,10 +433,14 @@ protected void handleTagCommand() {
String url = getUrl();
FullName name = new FullName(line);
String metalake = name.getMetalakeName();
- String tag = line.getOptionValue(GravitinoOptions.TAG);
-
+ String[] tags =
+
Arrays.stream(line.getOptionValues(GravitinoOptions.TAG)).distinct().toArray(String[]::new);
+ if (!validateMultipleTagOption(tags)) {
+ System.err.println(ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return;
+ }
if (CommandActions.DETAILS.equals(command)) {
- new TagDetails(url, ignore, metalake, tag).handle();
+ new TagDetails(url, ignore, metalake, tags[0]).handle();
Review Comment:
There is no guarantee that tags is not empty.
##########
docs/cli.md:
##########
@@ -456,10 +457,10 @@ gcli group delete --group new_group
gcli tag details --tag tagA
```
-#### Create a tag
+#### Create tags
```bash
- gcli tag create --tag tagA
+ gcli tag create --tag tagA tagB
Review Comment:
I don't like this syntax ...
It could have been `gcli tags create tagA tagB`, i.e. `--tag` is not
necessary.
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -444,44 +449,66 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- new CreateTag(url, ignore, metalake, tag, comment).handle();
+ new CreateTag(url, ignore, metalake, tags, comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- new DeleteTag(url, ignore, force, metalake, tag).handle();
+ new DeleteTag(url, ignore, force, metalake, tags).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (name == null && property != null && value != null) {
- new SetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ if (tags.length > 1) {
Review Comment:
The check of tags length for SET can be optimized.
I.e. this situation can be checked in the util method?
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -28,52 +31,55 @@
public class DeleteTag extends Command {
protected final String metalake;
- protected final String tag;
+ protected final String[] tags;
protected final boolean force;
/**
- * Delete a tag.
+ * Delete tags.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The names of the tags.
*/
- public DeleteTag(String url, boolean ignoreVersions, boolean force, String
metalake, String tag) {
+ public DeleteTag(
+ String url, boolean ignoreVersions, boolean force, String metalake,
String[] tags) {
super(url, ignoreVersions);
this.force = force;
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
}
- /** Delete a tag. */
+ /** Delete tags. */
@Override
public void handle() {
- boolean deleted = false;
-
if (!AreYouSure.really(force)) {
return;
}
-
+ List<String> deleted = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- deleted = client.deleteTag(tag);
+ for (String tag : tags) {
+ if (client.deleteTag(tag)) {
+ deleted.add(tag);
+ }
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (NoSuchTagException err) {
System.err.println(ErrorMessages.UNKNOWN_TAG);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
- }
-
- if (deleted) {
- System.out.println(tag + " deleted.");
- } else {
- System.out.println(tag + " not deleted.");
+ } finally {
+ if (!deleted.isEmpty()) {
+ System.out.println(String.join(",", deleted) + " deleted.");
Review Comment:
```suggestion
System.out.println("Tags " + String.join(",", deleted) + "
deleted.");
```
##########
clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java:
##########
@@ -444,44 +449,66 @@ protected void handleTagCommand() {
}
} else if (CommandActions.CREATE.equals(command)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- new CreateTag(url, ignore, metalake, tag, comment).handle();
+ new CreateTag(url, ignore, metalake, tags, comment).handle();
} else if (CommandActions.DELETE.equals(command)) {
boolean force = line.hasOption(GravitinoOptions.FORCE);
- new DeleteTag(url, ignore, force, metalake, tag).handle();
+ new DeleteTag(url, ignore, force, metalake, tags).handle();
} else if (CommandActions.SET.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
-
if (name == null && property != null && value != null) {
- new SetTagProperty(url, ignore, metalake, tag, property,
value).handle();
+ if (tags.length > 1) {
+ System.err.println(ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR);
+ return;
+ }
+ new SetTagProperty(url, ignore, metalake, tags[0], property,
value).handle();
} else if (name != null && property == null && value == null) {
- new TagEntity(url, ignore, metalake, name, tag).handle();
+ new TagEntity(url, ignore, metalake, name, tags).handle();
} else {
System.err.println(ErrorMessages.INVALID_SET_COMMAND);
}
} else if (CommandActions.REMOVE.equals(command)) {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
if (property != null) {
- new RemoveTagProperty(url, ignore, metalake, tag, property).handle();
+ if (tags.length > 1) {
Review Comment:
here too
##########
docs/cli.md:
##########
@@ -468,22 +469,22 @@ gcli tag details --tag tagA
gcli tag list
```
-#### Delete a tag
+#### Delete tags
```bash
-gcli tag delete --tag tagA
+gcli tag delete --tag tagA tagB
```
-#### Add a tag to an entity
+#### Add tags to an entity
```bash
-gcli tag set --name catalog_postgres.hr --tag tagA
+gcli tag set --name catalog_postgres.hr --tag tagA tagB
Review Comment:
This is confusing too.
The `--tag` option for the `tag`/`tags` command is weird.
```suggestion
gcli tags set --name catalog_postgres.hr tagA tagB
```
Or a different approach, where we don't separate tags with spaces in command
line.
```suggestion
gcli set --name catalog_postgres.hr --tags tagA,tagB
```
Or yet another valid syntax, where the `--tag` option can be specified
multiple times.
```suggestion
gcli set --name catalog_postgres.hr --tag tagA --tag tagB
```
##########
docs/cli.md:
##########
@@ -44,6 +44,7 @@ The general structure for running commands with the Gravitino
CLI is `gcli entit
--rename <arg> new entity name
-s,--server Gravitino server version
-t,--tag <arg> tag name
+ -t,--tags <arg> List tag names
Review Comment:
These will be confusing ...
##########
clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java:
##########
@@ -28,52 +31,55 @@
public class DeleteTag extends Command {
protected final String metalake;
- protected final String tag;
+ protected final String[] tags;
protected final boolean force;
/**
- * Delete a tag.
+ * Delete tags.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions
match.
* @param force Force operation.
* @param metalake The name of the metalake.
- * @param tag The name of the tag.
+ * @param tags The names of the tags.
*/
- public DeleteTag(String url, boolean ignoreVersions, boolean force, String
metalake, String tag) {
+ public DeleteTag(
+ String url, boolean ignoreVersions, boolean force, String metalake,
String[] tags) {
super(url, ignoreVersions);
this.force = force;
this.metalake = metalake;
- this.tag = tag;
+ this.tags = tags;
}
- /** Delete a tag. */
+ /** Delete tags. */
@Override
public void handle() {
- boolean deleted = false;
-
if (!AreYouSure.really(force)) {
return;
}
-
+ List<String> deleted = new ArrayList<>();
try {
GravitinoClient client = buildClient(metalake);
- deleted = client.deleteTag(tag);
+ for (String tag : tags) {
+ if (client.deleteTag(tag)) {
+ deleted.add(tag);
+ }
+ }
} catch (NoSuchMetalakeException err) {
System.err.println(ErrorMessages.UNKNOWN_METALAKE);
- return;
} catch (NoSuchTagException err) {
System.err.println(ErrorMessages.UNKNOWN_TAG);
- return;
} catch (Exception exp) {
System.err.println(exp.getMessage());
- return;
- }
-
- if (deleted) {
- System.out.println(tag + " deleted.");
- } else {
- System.out.println(tag + " not deleted.");
+ } finally {
+ if (!deleted.isEmpty()) {
+ System.out.println(String.join(",", deleted) + " deleted.");
+ if (deleted.size() < tags.length) {
+ List<String> remaining = Arrays.asList(tags);
+ remaining.removeAll(deleted);
+ System.out.println(String.join(",", remaining) + " not deleted.");
Review Comment:
```suggestion
System.out.println("Tags " + String.join(",", remaining) + " not
deleted.");
```
--
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]
