Re: [PR] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean merged PR #6037: URL: https://github.com/apache/gravitino/pull/6037 -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
Abyss-lord commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2567313423 > There is a small formatting issue that's causing Spotless to fail. You'll can run `./gradlew :clients:cli:spotlessApply` to fix this. hi @justinmclean , I’ve finished updating the code. Please take a look at the PR again when you have time. -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2567244968 There is a small formatting issue that's causing Spotless to fail. You'll can run `./gradlew :clients:cli:spotlessApply` to fix this. -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
Abyss-lord commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2566823600 Hi @justinmclean @tengqm ,I’ve finished updating the code. Please take a look at the PR again when you have time. -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on code in PR #6037:
URL: https://github.com/apache/gravitino/pull/6037#discussion_r1900290235
##
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java:
##
@@ -94,6 +97,9 @@ public void handle() {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
} catch (NoSuchTableException err) {
exitWithError(ErrorMessages.UNKNOWN_TABLE);
+} catch (TagAlreadyAssociatedException tagAlreadyAssociatedException) {
+ exitWithError(
+ "[" + COMMA_JOINER.join(tags) + "]" + " are already associated with
" + name.getName());
Review Comment:
This is not entirely correct, as it could be a subset of those tags. Does
the exception include the tag name? I'd also drop the brackets.
--
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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on code in PR #6037:
URL: https://github.com/apache/gravitino/pull/6037#discussion_r1900290027
##
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java:
##
@@ -94,6 +97,9 @@ public void handle() {
exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
} catch (NoSuchTableException err) {
exitWithError(ErrorMessages.UNKNOWN_TABLE);
+} catch (TagAlreadyAssociatedException tagAlreadyAssociatedException) {
Review Comment:
no need for the long variable name here
--
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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
Abyss-lord commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2566157572 Hi @justinmclean , I’ve finished updating the code. Please take a look at the PR again when you have time. -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2566059059 Tags already act like this: - The tags on an object is maintained as a set, i.e. no duplicated items are allowed. - When updating the tags on an object, the set of new tags is union-ed with that of the existing ones. This means you cannot remove any existing tags by updating the object with an empty list. - Tags can only be removed explicitly. I'm unaware of the exact naming character limitations or maximum number of tags, but I assume users will develop their own conventions. -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
tengqm commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2566047755 Some more thoughts on this ... Maybe we should tighten the check when adding or updating the tags on an entity. There should be a protocol, somewhere in the docs and the code. For example, - An object can optionally have a list of unique tags, each of which is an opaque, single-word string. - The tags on an object is maintained as a set, i.e. no duplicated items are allowed. - When updating the tags on an object, the set of new tags is union-ed with that of the existing ones. This means you cannot remove any existing tags by updating the object with an empty list. - An object can have at most N tags. - Tags can only be removed explicitly. That is, you have to invoke the `removeTags` API or using the command line. - A tag must start with a letter, and it can contain digits and `.`, `_` or `-`. The maximum length of a tag is M characters. - ... -- 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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
tengqm commented on code in PR #6037:
URL: https://github.com/apache/gravitino/pull/6037#discussion_r1899847773
##
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java:
##
@@ -102,4 +110,33 @@ public void handle() {
System.out.println(entity + " now tagged with " + all);
}
+
+ private void checkTags(Table gTable) {
+String[] associatedTags = gTable.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Schema gSchema) {
+String[] associatedTags = gSchema.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Catalog gCatalog) {
+String[] associatedTags = gCatalog.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
Review Comment:
I wouldn't structure the code this way because this thin layer of
`checkTags` can/should be compressed into the `handle` method.
We may want to split the `handle` method for catalog, schema and table,
instead.
That method is growing and there are chunks of `if...else` in it.
By the way, I wouldn't call tags "redundant", you are checking if tags are
"duplicated". This is a nit though.
--
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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on code in PR #6037:
URL: https://github.com/apache/gravitino/pull/6037#discussion_r189983
##
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java:
##
@@ -102,4 +110,33 @@ public void handle() {
System.out.println(entity + " now tagged with " + all);
}
+
+ private void checkTags(Table gTable) {
+String[] associatedTags = gTable.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Schema gSchema) {
+String[] associatedTags = gSchema.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Catalog gCatalog) {
+String[] associatedTags = gCatalog.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkRedundantTags(String[] associatedTags) {
+List redundantTags =
Review Comment:
There is no real need to do this extra work as you can just check for the
exception. There is also the (rare and unlikely) possibility that a tag could
be added after this check, as this is not in a transaction.
--
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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
justinmclean commented on code in PR #6037:
URL: https://github.com/apache/gravitino/pull/6037#discussion_r1899841131
##
clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java:
##
@@ -102,4 +110,33 @@ public void handle() {
System.out.println(entity + " now tagged with " + all);
}
+
+ private void checkTags(Table gTable) {
+String[] associatedTags = gTable.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Schema gSchema) {
+String[] associatedTags = gSchema.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkTags(Catalog gCatalog) {
+String[] associatedTags = gCatalog.supportsTags().listTags();
+checkRedundantTags(associatedTags);
+ }
+
+ private void checkRedundantTags(String[] associatedTags) {
+List redundantTags =
+Arrays.stream(associatedTags)
+.filter(x -> Arrays.asList(tags).contains(x))
+.collect(Collectors.toList());
+if (!redundantTags.isEmpty())
+ exitWithError(
+ "["
+ + COMMA_JOINER.join(redundantTags)
+ + "]"
+ + " are(is) already associated with "
Review Comment:
Please don't use "are(is)"
--
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] [#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output [gravitino]
Abyss-lord commented on PR #6037: URL: https://github.com/apache/gravitino/pull/6037#issuecomment-2565104682 Hi @justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback. -- 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]
