apache
[#6076] improve(CLI): Support model pre event to Gravitino server
What changes were proposed in this pull request?
Support model pre event to Gravitino server, based on #6129 , Both synchronize Dispatcher changes with each other
https://docs.google.com/document/d/1_aVCd_tKiEebpzp9tg07Lzdk1j6EalNn8YOIdRn-3z4/edit?tab=t.0#heading=h.k85t4bueowc5
PreEvent
| PreEvent | OperationType | ModelCatalog |
|---|---|---|
RegisterModelPreEvent |
REGISTER_MODEL |
registerModel |
GetModelPreEvent |
LOAD_MODEL |
getModel |
DeleteModelEvent |
Delete_MODEL |
deleteModel |
ListModelPreEvent |
LIST_MODEL |
listModels |
LinkModelVersionPreEvent |
LINK_MODEL_VERSION |
linkModelVersion |
GetModelVersionPreEvent |
GET_MODEL_VERSION |
getModelVersion |
DeleteModelVersionPreEvent |
Delete_MODEL_VERSION |
deleteModelVersion |
ListModelVersionsPreEvent |
LIST_MODEL_VERSIONS |
listModelVersions |
ModelEventDispatcher
Why are the changes needed?
Fix: #6076
Does this PR introduce any user-facing change?
No
How was this patch tested?
Model Event
testRegisterModelEventtestGetModelEventtestDeleteExistsModelEventtestDeleteNotExistsModelEventtestListModelEvent
Model Version Event
testLinkModelVersionEventtestGetModelVersionEventViaVersiontestGetModelVersionEventViaAliastestDeleteModelVersionEventViaVersiontestDeleteModelVersionEventViaAliastestDeleteModelVersionEventViaVersionNotExiststestListModelVersionsEvent
@FANNG1 @xunliu @orenccl could you please review this PR when you have time? I’d really appreciate your feedback.
@FANNG1 should we add ModelEventDispatcher to the Context in this pr? Or wait until all the events are complete？
@Abyss-lord sorry for the delay, could you check the comments and add document in https://github.com/apache/gravitino/blob/main/docs/gravitino-server-config.md?plain=1#L129 ?
Just took a quick glance over the eventing code base ... and I got a question. Given that we want to emit events for every operation on every entity, the current case-by-case implementation is problematic. It doesn't scale well.
Driven by this observation, I'm thinking if this can be drastically simplified. For example, we can use a handful of abstractions rather than tens of them to model events. Maybe we simply don't need all the fields in every event? I mean we can have a gemeroc link field pointing to the URI of the relevant object?
Just took a quick glance over the eventing code base ... and I got a question. Given that we want to emit events for every operation on every entity, the current case-by-case implementation is problematic. It doesn't scale well.
Driven by this observation, I'm thinking if this can be drastically simplified. For example, we can use a handful of abstractions rather than tens of them to model events. Maybe we simply don't need all the fields in every event? I mean we can have a gemeroc
linkfield pointing to the URI of the relevant object?
Yes, adding events one by one for all operations is painful. This is a trade-off, in this one-by-one way, the user could handle the event more flexibly, and if do with some general abstraction, we may lose some specific information in the event, any suggestions?
if do with some general abstraction, we may lose some specific information in the event, any suggestions?
It all depends on the use case, IMHO. We can always add "specific information" later on when needed. At this moment, we may want to rethink whether we really want to dump all details into events. If the event is about to be triggered for all requests and responses, we can have a nice abstraction, which is just to dump the request/response body. It is gonna be a heavy burden for such a detailed dump, especially for LIST operations.
Totally, please try to avoid doing another IO to get model or model version information for the event, using the parameter in the request seems enough.
@FANNG1 @tengqm I’ve finished updating the code and design doc. Please take a look at the PR again when you have time.
@FANNG1 It was my mistake that caused the problem？Why do so many workflows fail？
@FANNG1 It was my mistake that caused the problem？Why do so many workflows fail？
seems not related, the docker container is not started successfully.
@FANNG1 I’ve finished updating the code. Please take a look at the PR again when you have time.
Generally LGTM except minor comments, @jerryshao do you have time to review again?
@FANNG1 @jerryshao I’ve finished updating the code(add alias and version to GetModelVersionPreEvent ). Please take a look at the PR again when you have time.
@FANNG1 @jerryshao I’ve finished updating the code. Please take a look at the PR again when you have time.
merged to main, thanks @Abyss-lord for your contribution and @tengqm for the review.