[#4309] feat(core): support tag events for event listener #5847
Conversation
TungYuChiang
changed the title
| // TODO: getTagFailureEvent | ||
| throw e; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
I think TagEventDispatcher should forward requests to the TagManager?
Something like return dispathcer.getTag(metalake, name)
There was a problem hiding this comment.
+1, if not forwarding request, the tag request will takes no effect after this PR is merged.
|
suggest not moving |
|
The architecture looks good to me. |
|
|
||
| import org.apache.gravitino.annotation.Evolving; | ||
| import org.apache.gravitino.exceptions.NoSuchTagException; | ||
| import org.apache.gravitino.exceptions.TagAlreadyAssociatedException; | ||
| import org.apache.gravitino.tag.Tag; |
There was a problem hiding this comment.
Why do we do such changes in the API level? It will basically break the compatibility. If there's no strong reason, we should not do a such change.
There was a problem hiding this comment.
If we do not move the Supports in the API module, it will conflict with the one in the core module. I referred to other designs under the API, such as catalog, and moved it out of the Tag folder. Would you happen to have any good suggestions?
There was a problem hiding this comment.
I think you can have two options:
- No need to use dispatcher pattern, just implement the listener logic in
TagManager. - To refer to
PatitionDispatcher, just add the methods in thisTagDispatcherinterface, no need to have anotherSupportsXXXinterface.
There was a problem hiding this comment.
Hi Jerry,
I decided to go with Option 2 and have completed the implementation.
|
TungYuChiang
force-pushed
the
feature/tag_system
branch
2 times, most recently
from
630132f to
4e2acad
Compare
TungYuChiang
changed the title
| String[] listTagsForMetadataObject(String metalake, MetadataObject metadataObject); | ||
|
|
||
| /** | ||
| * @param metalake |
There was a problem hiding this comment.
please complement the java doc
| @@ -485,5 +489,6 @@ private void initGravitinoServerComponents() { | |||
|
|
|||
| // Tag manager | |||
| this.tagManager = new TagManager(idGenerator, entityStore); | |||
| this.tagDispatcher = new TagEventDispatcher(eventBus, tagManager); | |||
There was a problem hiding this comment.
There is no need to keep tagManager as a member of GravitinoEnv class?
| // TODO: getTagFailureEvent | ||
| throw e; | ||
| } | ||
| return null; |
There was a problem hiding this comment.
+1, if not forwarding request, the tag request will takes no effect after this PR is merged.
TungYuChiang
force-pushed
the
feature/tag_system
branch
from
4e2acad to
e57f980
Compare
TungYuChiang
force-pushed
the
feature/tag_system
branch
from
e57f980 to
6a33ebb
Compare
TungYuChiang
force-pushed
the
feature/tag_system
branch
from
6652303 to
adb7c97
Compare
|
@FANNG1 All comments have been addressed, and changes have been pushed. Please take another look when convenient. Thanks for the thorough review! |
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java
Outdated
Show resolved
Hide resolved
|
LGTM, @TungYuChiang do you like to add tag event in this PR or create a new PR? |
|
@FANNG1 I prefer create a new PR, I will open a new issue and create a few subtasks so that others can participate |
What changes were proposed in this pull request?
support tag events for event listener
Why are the changes needed?
Fix: #4309
Does this PR introduce any user-facing change?
no
How was this patch tested?
existing tests