[#5902] feat: Add tag failure event to Gravitino server #5944

cool9850311 · 2024-12-23T01:57:59Z

What changes were proposed in this pull request?

Add tag failure event to Gravitino server

Why are the changes needed?

Subtask: #5902

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests.

### What changes were proposed in this pull request? Add tag failure event to Gravitino server ### Why are the changes needed? Subtask: apache#5902 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests.

core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java

FANNG1 · 2024-12-23T03:51:54Z

core/src/main/java/org/apache/gravitino/listener/api/event/TagFailureEvent.java

+
+public class TagFailureEvent extends FailureEvent {
+    public TagFailureEvent(String user, Exception exception) {
+        super(user, null, exception);


identifier should not be null for tag event.

What should I pass here?

FANNG1 · 2024-12-23T03:53:00Z

core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java

+  DELETE_TAG,
+  ALTER_TAG,
+  LIST_TAG,
+  TAG_EXISTS,


TAG_EXISTS not used? please remove it

FANNG1 · 2024-12-23T03:55:29Z

core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java

      throw e;
    }
  }

  @Override
  public Tag createTag(
      String metalake, String name, String comment, Map<String, String> properties) {
+      MetalakeInfo metalakeInfo = new MetalakeInfo(name, comment, properties, null);


we should create a TagInfo not MetalakeInfo here

@SuppressWarnings

- Removed @SuppressWarnings annotations. - Removed unused OperationType. - Replaced MetalakeInfo with TagInfo. - Refactored TagFailureEvent constructor.

FANNG1 · 2024-12-23T11:06:58Z

core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java

@@ -7,7 +7,7 @@ public class AlterTagFailureEvent extends TagFailureEvent {
    private final String name;
    private final TagChange[] changes;
    public AlterTagFailureEvent(String user, String metalake, String name, TagChange[] changes, Exception exception) {
-        super(user, exception);
+        super(user, null, exception);


please provide a meaning identifier to all failure event, for tag operations the resource identifier is the $metalake.$tagName, for list operations the the resource identifier is the objects which supports list tags, such as $metalake.$metadataObject in listTagsForMetadataObject

Do I need to replace all String metalake in the TagDispatcher interface, as well as the fields that have been replaced with NameIdentifier in the TagManager, with NameIdentifier and construct the NameIdentifier in operations instead?

what do you mean about Do I need to replace all String metalake in the TagDispatcher interface?

I mean, currently TagDispatcher's methods are passing metalake as a String, and then turn it to NameIdentifier in TagManager.
I'm asking about if I should turn it to NameIdentifier earlier in TagOperations and then pass it through event and manager.

I perfer not changing the interfaces in TagManager

FANNG1 · 2024-12-25T07:28:42Z

event describes what happened to a resource identified by a nameIdentifier,

for most tag event(create/get/delete), the identifier is the metalakeName.tagName which could be generated by ofTagIdent(String metalake, String tagName) ,
list tag event, the identifier the resource which supports list tags like metalake, or metadata object
for tag event with metadataObject, the identifier is the $metalake.$metadataObject

FANNG1 · 2024-12-25T07:45:03Z

core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java

+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.tag.TagChange;
+


please add @DeveloperApi and simple java doc to all tag failure event, please refer to SchemaFailureEvent

...java/org/apache/gravitino/listener/api/event/AssociateTagsForMetadataObjectFailureEvent.java

xunliu · 2024-12-26T12:41:43Z

hi @FANNG1 Please help review this PR, thanks.

FANNG1 · 2024-12-27T07:53:31Z

core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java

+   */
+  public AlterTagFailureEvent(
+      String user, String metalake, String name, TagChange[] changes, Exception exception) {
+    super(user, TagManager.ofTagIdent(metalake, name), exception);


It's odd to refer TagManager here, how about move ofTagIdent to NameIdentifierUtil?

FANNG1 · 2024-12-27T07:55:40Z

core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java

+   *
+   * @return the name of the tag
+   */
+  public String name() {


please remove name() and metalake(), because they are not temp variables to generate tag identifier

FANNG1 · 2024-12-27T07:56:09Z

...java/org/apache/gravitino/listener/api/event/AssociateTagsForMetadataObjectFailureEvent.java

+   *
+   * @return The metadata object.
+   */
+  public MetadataObject metadataObject() {


remove metadataObject and metalake too

please check other events

FANNG1 · 2024-12-27T07:58:33Z

Could you fix the comments and could you add related document in gravitino-server-config.md?

cool9850311 · 2024-12-27T08:40:48Z

Could you fix the comments and could you add related document in gravitino-server-config.md?

OK

This commit cleans up the TagFailureEvent classes by removing the unused 'metalake', 'name' and 'metadataObject'fields, along with their associated methods. This refactoring simplifies the code and enhances maintainability.

FANNG1 reviewed Dec 23, 2024

View reviewed changes

Refactor codebase:

21aa71e

- Removed @SuppressWarnings annotations. - Removed unused OperationType. - Replaced MetalakeInfo with TagInfo. - Refactored TagFailureEvent constructor.

FANNG1 reviewed Dec 23, 2024

View reviewed changes

Add meaning identifier to all failure event

458a091

FANNG1 mentioned this pull request Dec 25, 2024

[#5900] feat(tag): support tag pre-event to Gravitino server #5980

Draft

FANNG1 reviewed Dec 25, 2024

View reviewed changes

...java/org/apache/gravitino/listener/api/event/AssociateTagsForMetadataObjectFailureEvent.java Show resolved Hide resolved

cool9850311 added 5 commits December 25, 2024 21:15

Change ident to metalake & tag name

9a33cf1

Add javadoc to all tag failure event

250a977

Format code

259295b

Add docs and license

f73787d

format code

4ccbe90

FANNG1 mentioned this pull request Dec 26, 2024

[#5901] feat(core): support tag event to Gravitino server #5998

Draft

cool9850311 marked this pull request as ready for review December 27, 2024 02:25

FANNG1 reviewed Dec 27, 2024

View reviewed changes

cool9850311 added 3 commits December 27, 2024 20:22

refactor: move tag methods to util

572a823

refactor: remove unused fields from tag failure events

7341ee5

This commit cleans up the TagFailureEvent classes by removing the unused 'metalake', 'name' and 'metadataObject'fields, along with their associated methods. This refactoring simplifies the code and enhances maintainability.

update readme

9410eb5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5902] feat: Add tag failure event to Gravitino server #5944

[#5902] feat: Add tag failure event to Gravitino server #5944

cool9850311 commented Dec 23, 2024

FANNG1 Dec 23, 2024

cool9850311 Dec 23, 2024

FANNG1 Dec 23, 2024

FANNG1 Dec 23, 2024

FANNG1 Dec 23, 2024

cool9850311 Dec 23, 2024

cool9850311 Dec 24, 2024

FANNG1 Dec 24, 2024

cool9850311 Dec 24, 2024

FANNG1 Dec 24, 2024

cool9850311 Dec 24, 2024

FANNG1 commented Dec 25, 2024

FANNG1 Dec 25, 2024

xunliu commented Dec 26, 2024

FANNG1 Dec 27, 2024

FANNG1 Dec 27, 2024

FANNG1 Dec 27, 2024

FANNG1 Dec 27, 2024

FANNG1 commented Dec 27, 2024 •

edited

Loading

cool9850311 commented Dec 27, 2024


		import org.apache.gravitino.NameIdentifier;
		import org.apache.gravitino.tag.TagChange;

[#5902] feat: Add tag failure event to Gravitino server #5944

Are you sure you want to change the base?

[#5902] feat: Add tag failure event to Gravitino server #5944

Conversation

cool9850311 commented Dec 23, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FANNG1 commented Dec 25, 2024

Choose a reason for hiding this comment

xunliu commented Dec 26, 2024

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

FANNG1 commented Dec 27, 2024 • edited Loading

cool9850311 commented Dec 27, 2024

FANNG1 commented Dec 27, 2024 •

edited

Loading