Re: [PR] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2025-01-07 Thread via GitHub


jerryshao merged PR #5944:
URL: https://github.com/apache/gravitino/pull/5944


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2025-01-06 Thread via GitHub


FANNG1 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2574487181

   LGTM, @jerryshao do you have time to review again?


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2025-01-06 Thread via GitHub


FANNG1 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2574267922

   @cool9850311 , LGTM except for minor comments, could you fix it?


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2025-01-06 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1904819488


##
core/src/main/java/org/apache/gravitino/listener/api/event/ListMetadataObjectsForTagFailureEvent.java:
##
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.DeveloperApi;
+
+/**
+ * Represents an event triggered when an attempt to list metadata objects for 
a tag fails due to an
+ * exception.
+ */
+@DeveloperApi
+public class ListMetadataObjectsForTagFailureEvent extends TagFailureEvent {
+  /**
+   * Constructs a new {@code ListMetadataObjectsForTagFailureEvent} instance.
+   *
+   * @param user The user who initiated the operation.
+   * @param metalake The metalake name where the tag resides.
+   * @param name The name of the tag.
+   * @param exception The exception encountered during the operation, 
providing insights into the
+   * reasons behind the failure.
+   */
+  public ListMetadataObjectsForTagFailureEvent(
+  String user, String metalake, String name, Exception exception) {
+super(user, NameIdentifier.of(metalake), exception);

Review Comment:
   please use `NameIdentifierUtil.ofTag(metalake, name)`



##
core/src/main/java/org/apache/gravitino/listener/api/event/TagFailureEvent.java:
##
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.annotation.DeveloperApi;
+
+/**
+ * Represents an event triggered when an attempt to perform a tag operation 
fails due to an
+ * exception.
+ */
+@DeveloperApi
+public class TagFailureEvent extends FailureEvent {

Review Comment:
   make it abstract class?



##
docs/gravitino-server-config.md:
##
@@ -124,6 +124,7 @@ Gravitino triggers a pre-event before the operation, a 
post-event after the comp
 | catalog operation   | `CreateCatalogEvent`, 
`AlterCatalogEvent`, `DropCatalogEvent`, `LoadCatalogEvent`, 
`ListCatalogEvent`, `CreateCatalogFailureEvent`, `AlterCatalogFailureEvent`, 
`DropCatalogFailureEvent`, `LoadCatalogFailureEvent`, `ListCatalogFailureEvent` 


  | 0.5.0|
 | metalake operation  | `CreateMetalakeEvent`, 
`AlterMetalakeEvent`, `DropMetalakeEvent`, `LoadMetalakeEvent`, 
`ListMetalakeEvent`, `CreateMetalakeFailureEvent`, `AlterMetalakeFailureEvent`, 
`DropMetalakeFailureEvent`, `LoadMetalakeFailureEvent`, 
`ListMetalakeFailureEvent`  

   | 0.5.0|
 | Iceberg REST server table operation | `IcebergCreateTableEvent`, 
`IcebergUpdateTableEvent`, `IcebergDropTableEvent`, `IcebergLoadTableEvent`, 
`IcebergListTableEvent`, `IcebergTableExistsEvent`, `IcebergRenameTableEvent`, 
`IcebergCreateTableFailureEvent`, `IcebergUpdateTableFailureEvent`, 
`IcebergDropTableFailureEvent`, `IcebergLoadTableFailureEvent`, 
`IcebergListTableFailureEvent`, `IcebergRenameTableFailureEvent`, 
`Iceberg

Re: [PR] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-28 Thread via GitHub


FANNG1 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2564572784

   > @FANNG1 I run the Frontend IT test successfully in local, but it does not 
pass here 2 times, and it says connection timeout, do I need to change anything?
   
   no, it's network failures, reruned it.


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-28 Thread via GitHub


cool9850311 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2564371371

   @FANNG1 I run the Frontend IT test successfully in local, do I need to 
change anything?


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-27 Thread via GitHub


cool9850311 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2564159266

   @FANNG1 Done, pls review thx


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-27 Thread via GitHub


cool9850311 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2563464709

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


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-26 Thread via GitHub


FANNG1 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2563431271

   Could you add related document in `gravitino-server-config.md`?


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-26 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1898338425


##
core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java:
##
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.tag.TagChange;
+import org.apache.gravitino.tag.TagManager;
+
+/**
+ * Represents an event triggered when an attempt to alter a tag in the 
database fails due to an
+ * exception.
+ */
+@DeveloperApi
+public class AlterTagFailureEvent extends TagFailureEvent {
+  private final String metalake;
+  private final String name;
+  private final TagChange[] changes;
+
+  /**
+   * Constructs a new AlterTagFailureEvent.
+   *
+   * @param user the user who attempted to alter the tag
+   * @param metalake the metalake identifier
+   * @param name the name of the tag
+   * @param changes the changes attempted to be made to the tag
+   * @param exception the exception that caused the failure
+   */
+  public AlterTagFailureEvent(
+  String user, String metalake, String name, TagChange[] changes, 
Exception exception) {
+super(user, TagManager.ofTagIdent(metalake, name), exception);

Review Comment:
   It's odd to refer `TagManager` here, how about move `ofTagIdent` to 
`NameIdentifierUtil`?



##
core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java:
##
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.tag.TagChange;
+import org.apache.gravitino.tag.TagManager;
+
+/**
+ * Represents an event triggered when an attempt to alter a tag in the 
database fails due to an
+ * exception.
+ */
+@DeveloperApi
+public class AlterTagFailureEvent extends TagFailureEvent {
+  private final String metalake;
+  private final String name;
+  private final TagChange[] changes;
+
+  /**
+   * Constructs a new AlterTagFailureEvent.
+   *
+   * @param user the user who attempted to alter the tag
+   * @param metalake the metalake identifier
+   * @param name the name of the tag
+   * @param changes the changes attempted to be made to the tag
+   * @param exception the exception that caused the failure
+   */
+  public AlterTagFailureEvent(
+  String user, String metalake, String name, TagChange[] changes, 
Exception exception) {
+super(user, TagManager.ofTagIdent(metalake, name), exception);
+this.name = name;
+this.metalake = metalake;
+this.changes = changes;
+  }
+
+  /**
+   * Returns the name of the tag.
+   *
+   * @return the name of the tag
+   */
+  public String name() {

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



##
core/src/main/java/org/apache/gravitino/listener/api/event/AssociateTagsForMetadataObjectFailureEvent.java:
##
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you m

Re: [PR] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-26 Thread via GitHub


xunliu commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2562645309

   hi @FANNG1 Please help review this PR, thanks.


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-24 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1897174816


##
core/src/main/java/org/apache/gravitino/listener/api/event/AlterTagFailureEvent.java:
##
@@ -0,0 +1,33 @@
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.tag.TagChange;
+

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



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-24 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1897174971


##
core/src/main/java/org/apache/gravitino/listener/api/event/AssociateTagsForMetadataObjectFailureEvent.java:
##
@@ -0,0 +1,38 @@
+package org.apache.gravitino.listener.api.event;
+
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.utils.MetadataObjectUtil;

Review Comment:
   please format code `./gradlew spotlessApply`



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-24 Thread via GitHub


FANNG1 commented on PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#issuecomment-2561680979

   event describes what happened to a resource identified by a nameIdentifier, 
   1. for most tag event(create/get/delete), the identifier is the 
`metalakeName`.`tagName` which could be generated by `ofTagIdent(String 
metalake, String tagName)` ,
   2.  list tag event, the identifier the resource which supports list tags 
like metalake, or metadata object
   3. for tag event with metadataObject, the identifier is the  
$metalake.$metadataObject


-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


cool9850311 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1896461209


##
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);

Review Comment:
   Ok



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1896439734


##
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);

Review Comment:
   I perfer not changing the interfaces in TagManager



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


cool9850311 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1896365535


##
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);

Review Comment:
   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.



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1896291754


##
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);

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



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


cool9850311 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1896250583


##
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);

Review Comment:
   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?



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


cool9850311 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1895749129


##
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);

Review Comment:
   Ok



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-23 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1895619541


##
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);

Review Comment:
   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`



-- 
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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-22 Thread via GitHub


cool9850311 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1895371248


##
core/src/main/java/org/apache/gravitino/listener/api/event/TagFailureEvent.java:
##
@@ -0,0 +1,7 @@
+package org.apache.gravitino.listener.api.event;
+
+public class TagFailureEvent extends FailureEvent {
+public TagFailureEvent(String user, Exception exception) {
+super(user, null, exception);

Review Comment:
   What should I pass 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] [#5902] feat: Add tag failure event to Gravitino server [gravitino]

2024-12-22 Thread via GitHub


FANNG1 commented on code in PR #5944:
URL: https://github.com/apache/gravitino/pull/5944#discussion_r1895209349


##
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java:
##
@@ -19,11 +19,25 @@
 package org.apache.gravitino.listener;
 
 import java.util.Map;
+
 import org.apache.gravitino.MetadataObject;
 import org.apache.gravitino.exceptions.NoSuchTagException;
+import org.apache.gravitino.listener.api.event.AlterTagFailureEvent;
+import 
org.apache.gravitino.listener.api.event.AssociateTagsForMetadataObjectFailureEvent;
+import org.apache.gravitino.listener.api.event.CreateTagFailureEvent;
+import org.apache.gravitino.listener.api.event.DeleteTagFailureEvent;
+import org.apache.gravitino.listener.api.event.GetTagFailureEvent;
+import 
org.apache.gravitino.listener.api.event.GetTagForMetadataObjectFailureEvent;
+import 
org.apache.gravitino.listener.api.event.ListMetadataObjectsForTagFailureEvent;
+import org.apache.gravitino.listener.api.event.ListTagFailureEvent;
+import 
org.apache.gravitino.listener.api.event.ListTagsForMetadataObjectFailureEvent;
+import org.apache.gravitino.listener.api.event.ListTagsInfoFailureEvent;
+import 
org.apache.gravitino.listener.api.event.ListTagsInfoForMetadataObjectFailureEvent;
+import org.apache.gravitino.listener.api.info.MetalakeInfo;
 import org.apache.gravitino.tag.Tag;
 import org.apache.gravitino.tag.TagChange;
 import org.apache.gravitino.tag.TagDispatcher;
+import org.apache.gravitino.utils.PrincipalUtils;
 

Review Comment:
   please remove `@SuppressWarnings("unused")` in TagEventDispatcher
   ```
 @SuppressWarnings("unused")
 private final EventBus eventBus;
   ```



##
core/src/main/java/org/apache/gravitino/listener/api/event/TagFailureEvent.java:
##
@@ -0,0 +1,7 @@
+package org.apache.gravitino.listener.api.event;
+
+public class TagFailureEvent extends FailureEvent {
+public TagFailureEvent(String user, Exception exception) {
+super(user, null, exception);

Review Comment:
   identifier should not be null for tag event. 



##
core/src/main/java/org/apache/gravitino/listener/api/event/OperationType.java:
##
@@ -31,6 +31,20 @@ public enum OperationType {
   REGISTER_TABLE,
   TABLE_EXISTS,
 
+  // Tag operations
+  CREATE_TAG,
+  GET_TAG,
+  GET_TAG_FOR_METADATA_OBJECT,
+  DELETE_TAG,
+  ALTER_TAG,
+  LIST_TAG,
+  TAG_EXISTS,

Review Comment:
   `TAG_EXISTS` not used? please remove it



##
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java:
##
@@ -73,21 +87,22 @@ public Tag getTag(String metalake, String name) throws 
NoSuchTagException {
 try {
   // TODO: getTagEvent
   return dispatcher.getTag(metalake, name);
-} catch (NoSuchTagException e) {
-  // TODO: getTagFailureEvent
+} catch (Exception e) {
+  eventBus.dispatchEvent(new 
GetTagFailureEvent(PrincipalUtils.getCurrentUserName(), metalake, name, e));
   throw e;
 }
   }
 
   @Override
   public Tag createTag(
   String metalake, String name, String comment, Map 
properties) {
+  MetalakeInfo metalakeInfo = new MetalakeInfo(name, comment, properties, 
null);

Review Comment:
   we should create a `TagInfo` not `MetalakeInfo` 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]