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

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

Merged
merged 15 commits into from
Mar 3, 2025
Merged

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

merged 15 commits into from
Mar 3, 2025

Conversation

TungYuChiang
Copy link
Contributor

@TungYuChiang TungYuChiang commented Dec 26, 2024 •
edited
Loading

What changes were proposed in this pull request?

  1. Create corresponding event like ListTagEvent
  2. Add corresponding event to TagEventDispatcher

Why are the changes needed?

Fix: #5901

Does this PR introduce any user-facing change?

no

How was this patch tested?

@orenccl orenccl requested a review from FANNG1 December 26, 2024 03:05
@orenccl
Copy link
Collaborator

orenccl commented Dec 26, 2024

It would be better to turn this PR into a draft before updating the unit tests.

@xunliu xunliu marked this pull request as draft December 26, 2024 08:26
@FANNG1
Copy link
Contributor

FANNG1 commented Dec 26, 2024

will review the PR until #5944 is merged for some duplicated logic, please wait for a while.

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 7, 2025

@TungYuChiang could you rebase your code since #5944 is merged

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 7, 2025

cc @cool9850311

@TungYuChiang
Copy link
Contributor Author

@TungYuChiang could you rebase your code since #5944 is merged

sure, I will complete it within the next few days.

@TungYuChiang TungYuChiang force-pushed the feature/tagEvent branch 4 times, most recently from a680fe6 to 43c2d20 Compare January 11, 2025 08:26
@FANNG1
Copy link
Contributor

FANNG1 commented Jan 13, 2025

Event represents an specific operation to specific resource, the current implement pass metalake as resource name, this's not correct for some event, please refer to XXTagFailureEvent for the definition of the resource.

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 13, 2025

please update the document too

@orenccl orenccl mentioned this pull request Jan 13, 2025
Closed
@FANNG1
Copy link
Contributor

FANNG1 commented Jan 15, 2025

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@TungYuChiang
Copy link
Contributor Author

TungYuChiang commented Jan 15, 2025 •
edited
Loading

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@FANNG1 , I’m sorry for the delay. I’ve been a bit busy these past few days, but I’ll finish it by tomorrow evening.
Thank you for your patience

@FANNG1
Copy link
Contributor

FANNG1 commented Jan 15, 2025

@TungYuChiang any progress? If this PR is merged these days, we could meet the 0.8 release.

@FANNG1 , I’m sorry for the delay. I’ve been a bit busy these past few days, but I’ll finish it by tomorrow evening. Thank you for your patience

no hurry, please take your time

@TungYuChiang
Copy link
Contributor Author

@FANNG1, I have fixed everything except renaming ListTagsInfoFailureEvent to ListTagInfoFailureEvent

@TungYuChiang TungYuChiang force-pushed the feature/tagEvent branch 3 times, most recently from 9740fcb to 64a293c Compare January 19, 2025 08:13
@TungYuChiang
Copy link
Contributor Author

@FANNG1, I have fixed everything except renaming ListTagsInfoFailureEvent to ListTagInfoFailureEvent

@TungYuChiang TungYuChiang reopened this Jan 19, 2025
@TungYuChiang
Copy link
Contributor Author

@FANNG1 , Thank you for your review!
I've addressed all the comments.

@FANNG1
Copy link
Contributor

FANNG1 commented Feb 24, 2025

LGTM, except two minor comments

@TungYuChiang
Add TagEvent
bf5d983
@TungYuChiang
Add Create & List TagEvent
f49920e
@TungYuChiang
Add all tag event
387ffa8
@TungYuChiang
spotlessApply
0ac37a3
@TungYuChiang
fix bugs
b89ae8d
@TungYuChiang
Fix bugs of AlterTagEvent
d471658
@TungYuChiang
ListTagEvent to ListTagsEvent & ListTagInfoEvent to ListTagsInfoEvent
66ee0b4
@TungYuChiang
- Updated event handling logic to use TagInfo:
ff92f46 
  - Modified `TagEventDispatcher` to pass `TagInfo` in relevant events.
  - Updated `ListTagsEvent`, `GetTagEvent`, and `GetTagForMetadataObjectEvent` classes to support `TagInfo`.
@TungYuChiang
-Refactor Events: Updated constructors in various event classes to us…
8667619 
…e NameIdentifierUtil.ofTag and MetadataObjectUtil.toEntityIdent for consistent identifier creation.
@TungYuChiang
-remove duplicated function
62845cd
@TungYuChiang
fix bugs of .clone()
325a826
@TungYuChiang
fix bugs
9e3c8bf
@TungYuChiang
rebase master & fix comment
087bf39
@TungYuChiang
fix comment
d3b1ec4
@TungYuChiang
docs: fix formatting in tag operation events table
c68e741
@TungYuChiang TungYuChiang requested a review from orenccl February 27, 2025 08:15
@FANNG1
Copy link
Contributor

FANNG1 commented Mar 3, 2025

@TungYuChiang , merged to main, thanks for your contribution, thanks @orenccl for your review!

@FANNG1 FANNG1 merged commit a5169f1 into apache:main Mar 3, 2025
28 checks passed
@TungYuChiang
Copy link
Contributor Author

@TungYuChiang , merged to main, thanks for your contribution, thanks @orenccl for your review!

Thanks for your review and help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FANNG1 FANNG1 FANNG1 approved these changes

@orenccl orenccl Awaiting requested review from orenccl

Assignees

@TungYuChiang TungYuChiang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Support tag event to Gravitino server
3 participants
@TungYuChiang @orenccl @FANNG1