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.

Sign up for GitHub

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

Jump to bottom

[#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output #6037

Merged
merged 5 commits into from
Jan 2, 2025
Merged

[#6030] fix(CLI): Fix Setting the same tags multiple times in the Gravitino CLi gives unexpected output #6037

merged 5 commits into from
Jan 2, 2025

Conversation

Abyss-lord
Copy link
Contributor

What changes were proposed in this pull request?

Fix the error information when Setting the same tags multiple times in the Gravitino CLi. now a hint information is given when the tag is set repeatedly

Why are the changes needed?

Fix: #6030

Does this PR introduce any user-facing change?

NO

How was this patch tested?

gcli tag set --metalake demo_metalake --name Hive_catalog --tag tagB tagC
# Hive_catalog now tagged with tagB,tagC

gcli tag set --metalake demo_metalake --name Hive_catalog --tag tagB tagC
# [tagB, tagC] are(is) already associated with Hive_catalog

@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
7dd31fa 
…he Gravitino CLi gives unexpected output

Fix the error information when Setting the same tags multiple times in the Gravitino CLi.
@Abyss-lord
Copy link
Contributor Author

Hi @justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback.

@tengqm
Copy link
Contributor

tengqm commented Dec 31, 2024

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.
  • ...

@justinmclean
Copy link
Member

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.

@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
4331e0d 
…he Gravitino CLi gives unexpected output

fix some error
@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Dec 31, 2024 •
edited
Loading

Hi @justinmclean @shaofengshi , I’ve finished updating the code. Please take a look at the PR again when you have time.

@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
61e1b1d 
…he Gravitino CLi gives unexpected output

fix some error, remove long error name and fix the error info.
@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
417afa1 
…he Gravitino CLi gives unexpected output

fix some error.
@Abyss-lord
Copy link
Contributor Author

Hi @justinmclean @tengqm ,I’ve finished updating the code. Please take a look at the PR again when you have time.

@justinmclean
Copy link
Member

There is a small formatting issue that's causing Spotless to fail. You'll can run ./gradlew :clients:cli:spotlessApply to fix this.

@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
d660c12 
…he Gravitino CLi gives unexpected output

fix some error.
@Abyss-lord
Copy link
Contributor Author

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.

justinmclean
justinmclean approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

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

LGTM

@justinmclean justinmclean merged commit fb75616 into apache:main Jan 2, 2025
25 checks passed
@Abyss-lord Abyss-lord deleted the fix-tag-entity branch January 3, 2025 01:07
Abyss-lord added a commit to Abyss-lord/gravitino that referenced this pull request Jan 3, 2025
@Abyss-lord
[apache#6030] fix(CLI): Fix Setting the same tags multiple times in t…
2221095 
…he Gravitino CLi gives unexpected output (apache#6037)

### What changes were proposed in this pull request?

Fix the error information when Setting the same tags multiple times in
the Gravitino CLi. now a hint information is given when the tag is set
repeatedly

### Why are the changes needed?

Fix: apache#6030 

### Does this PR introduce _any_ user-facing change?

NO

### How was this patch tested?

```bash
gcli tag set --metalake demo_metalake --name Hive_catalog --tag tagB tagC
# Hive_catalog now tagged with tagB,tagC

gcli tag set --metalake demo_metalake --name Hive_catalog --tag tagB tagC
# [tagB, tagC] are(is) already associated with Hive_catalog
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmclean justinmclean justinmclean approved these changes

@tengqm tengqm Awaiting requested review from tengqm

Assignees

@Abyss-lord Abyss-lord

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Improvement] Setting the same tags multiple times in the Gravitino CLi gives unexpected output
3 participants
@Abyss-lord @tengqm @justinmclean