[#3460] feat(api): Add API design for Tag system #3486

jerryshao · 2024-05-21T10:30:08Z

What changes were proposed in this pull request?

This PR adds the API support for Tag system in Gravitino.

Why are the changes needed?

This is the first step to add a tag system.

Fix: #3460

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

No. Test will be added later when implementing the logic.

api/src/main/java/com/datastrato/gravitino/SupportsCatalogs.java

api/src/main/java/com/datastrato/gravitino/SupportsMetalakes.java

shaofengshi · 2024-05-22T07:25:50Z

api/src/main/java/com/datastrato/gravitino/tag/SupportsTags.java

+   * @param tagsToRemove The array of tag name to be removed from the entity.
+   * @return The array of tag names that are associated with the entity.
+   */
+  String[] associateTags(String[] tagsToAdd, String[] tagsToRemove);


for this method, the requestor need to know which tags are new, which are existed but to be removed, this might be a little complicated for the requestor; how about change it to "applyTags(String[] tags)", so that each time the requestor pass in a full set of the tags, no need to distinguish which are new and which are to be remove;

From the current API, users still don't need to distinguish whether the tag is new or not. They can add all the tags they want to tagsToAdd, I will handle this in the backend.

I see, the backend will compare and recognize that, which is good for the client.

If a tag appears in two arrays at the same time, what will be the result?

It will be no-op. Same as if we both enable setProperty and removeProperty on the same key for alterTable.

jerqi · 2024-05-22T11:54:46Z

For snowflake, tag is related to the securable object. You can see https://docs.snowflake.com/en/user-guide/object-tagging

Tag lineage[¶](https://docs.snowflake.com/en/user-guide/object-tagging#tag-lineage)
A tag is inherited based on the Snowflake securable object hierarchy. Snowflake recommends defining the tag keys as closely as possible to the [securable object](https://docs.snowflake.com/en/user-guide/security-access-control-overview.html#label-access-control-securable-objects) hierarchy in your Snowflake environment.

mchades · 2024-05-23T03:19:03Z

api/src/main/java/com/datastrato/gravitino/tag/Tag.java

+    }
+
+    /** @return The number of entities that are using this tag. */
+    int inUse();


should this method have a default implementation?

return usedEntities().length

api/src/main/java/com/datastrato/gravitino/tag/Tag.java

jerryshao · 2024-05-23T07:06:27Z

@shaofengshi @jerqi @mchades can you please review again?

mchades

two comments left:
#3486 (comment)

https://github.com/datastrato/gravitino/pull/3486/files#r1610911198

shaofengshi · 2024-05-23T07:27:36Z

@shaofengshi @jerqi @mchades can you please review again?

LGTM

jerqi · 2024-05-23T07:42:57Z

api/src/main/java/com/datastrato/gravitino/tag/Tag.java

+    int inUse();
+
+    /** @return The list of objects that are using this tag. */
+    MetadataObject[] usedObjects();


How about associatedObjects?

jerqi · 2024-05-23T07:47:19Z

api/src/main/java/com/datastrato/gravitino/tag/Tag.java

+  /** The extended information of the tag. */
+  interface Extended {
+    /** @return The number of objects that are using this tag. */
+    default int inUse() {


inUse seems a state. Could you give this a better name?

jerqi · 2024-05-23T07:56:20Z

api/src/main/java/com/datastrato/gravitino/tag/TagOperations.java

+   * @throws NoSuchTagException If the tag does not exist.
+   * @throws IllegalArgumentException If the changes cannot be applied to the tag.
+   */
+  Tag alterTag(String name, TagChange... changes)


Should we use updateTag? Snowflake use AlterTag. Git uses UpdateTag. All is ok for me.

I'm OK with either, I have no strong preference.

jerqi · 2024-05-23T08:11:33Z

api/src/main/java/com/datastrato/gravitino/tag/TagOperations.java

+   * @return The tag.
+   * @throws NoSuchTagException If the tag does not exist.
+   */
+  Tag getTag(String name) throws NoSuchTagException;


Do we need extended?

Currently, there's no such requirement based on our UI requirement, I think we can add it later on.

jerryshao · 2024-05-23T08:13:08Z

I changed to inAssociation, WDYT? @jerqi

jerqi · 2024-05-23T09:38:28Z

I changed to inAssociation, WDYT? @jerqi

Extended's inAssocation may be confusing for users. Because Extended can't give any information about this is related to object. I may prefer associatedObjectsCount.
Or
we can have another abstraction layer like

Extened {
    AssociationObjects {
         count
         objects
   }
}

or we remove extended this layer.

    AssociationObjects {
         count
         objects
   }

Maybe I don't provide a enough proper solution, you can think a better solution.

jerryshao · 2024-05-23T09:59:57Z

Extended basically means that we have some extended information method for the tag, this may not be limited to the current two interfaces, we can add more later. While changing to AssociationObjects will limit the extensions, the reason why I use Extended is referred from Spark's explain API to add more information to the current object.

jerryshao · 2024-05-23T11:45:36Z

@jerqi please review again?

jerqi · 2024-05-23T11:49:46Z

api/src/main/java/com/datastrato/gravitino/tag/Tag.java

+  /** The interface of the associated objects of the tag. */
+  interface AssociatedObjects {
+
+    /** @return The number of objects that are associated with this Tag */


Tag -> tag.

I will fix this in the following PR.

jerqi

LGTM. Just one nit.

### What changes were proposed in this pull request? This PR adds the API support for Tag system in Gravitino. ### Why are the changes needed? This is the first step to add a tag system. Fix: apache#3460 ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? No. Test will be added later when implementing the logic.

jerryshao requested review from FANNG1, shaofengshi, yuqi1129 and mchades May 21, 2024 10:30

jerryshao self-assigned this May 21, 2024

shaofengshi reviewed May 22, 2024

View reviewed changes

api/src/main/java/com/datastrato/gravitino/SupportsCatalogs.java Outdated Show resolved Hide resolved

shaofengshi reviewed May 22, 2024

View reviewed changes

api/src/main/java/com/datastrato/gravitino/SupportsMetalakes.java Outdated Show resolved Hide resolved

shaofengshi reviewed May 22, 2024

View reviewed changes

shaofengshi previously approved these changes May 22, 2024

View reviewed changes

mchades reviewed May 23, 2024

View reviewed changes

api/src/main/java/com/datastrato/gravitino/tag/Tag.java Show resolved Hide resolved

mchades reviewed May 23, 2024

View reviewed changes

api/src/main/java/com/datastrato/gravitino/tag/Tag.java Outdated Show resolved Hide resolved

jerryshao added 3 commits May 23, 2024 15:01

Add API design for Tag system

56d790e

Add javadoc for the APIs

5d7724b

abstract a metadata object interface to be used for tag and auth

deae5e6

jerryshao dismissed shaofengshi’s stale review via deae5e6 May 23, 2024 07:01

jerryshao force-pushed the issue-3460 branch from 45a4a55 to deae5e6 Compare May 23, 2024 07:01

mchades reviewed May 23, 2024

View reviewed changes

shaofengshi previously approved these changes May 23, 2024

View reviewed changes

address the comments

adc46ed

jerryshao dismissed shaofengshi’s stale review via adc46ed May 23, 2024 07:42

jerqi reviewed May 23, 2024

View reviewed changes

address the comments

13b14dc

jerqi reviewed May 23, 2024

View reviewed changes

Address the comments

0039e2d

jerqi reviewed May 23, 2024

View reviewed changes

jerqi approved these changes May 23, 2024

View reviewed changes

qqqttt123 approved these changes May 23, 2024

View reviewed changes

jerryshao merged commit 29a3d62 into apache:main May 23, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3460] feat(api): Add API design for Tag system #3486

[#3460] feat(api): Add API design for Tag system #3486

jerryshao commented May 21, 2024

shaofengshi May 22, 2024

jerryshao May 22, 2024

shaofengshi May 22, 2024

mchades May 23, 2024

jerryshao May 23, 2024 •

edited

Loading

jerqi commented May 22, 2024

mchades May 23, 2024

jerryshao May 23, 2024

jerryshao commented May 23, 2024

mchades left a comment

shaofengshi commented May 23, 2024

jerqi May 23, 2024

jerryshao May 23, 2024

jerqi May 23, 2024

jerqi May 23, 2024 •

edited

Loading

jerryshao May 23, 2024

jerqi May 23, 2024

jerryshao May 23, 2024

jerryshao commented May 23, 2024

jerqi commented May 23, 2024 •

edited

Loading

jerryshao commented May 23, 2024 •

edited

Loading

jerryshao commented May 23, 2024

jerqi May 23, 2024

jerryshao May 23, 2024

jerqi left a comment

[#3460] feat(api): Add API design for Tag system #3486

[#3460] feat(api): Add API design for Tag system #3486

Conversation

jerryshao commented May 21, 2024

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao May 23, 2024 • edited Loading

Choose a reason for hiding this comment

jerqi commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 23, 2024

mchades left a comment

Choose a reason for hiding this comment

shaofengshi commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented May 23, 2024

jerqi commented May 23, 2024 • edited Loading

jerryshao commented May 23, 2024 • edited Loading

jerryshao commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

jerryshao May 23, 2024 •

edited

Loading

jerqi May 23, 2024 •

edited

Loading

jerqi commented May 23, 2024 •

edited

Loading

jerryshao commented May 23, 2024 •

edited

Loading