-
Notifications
You must be signed in to change notification settings - Fork 60
feat(collections): add annotations for handling collections #571
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.
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
base: master
Are you sure you want to change the base?
Conversation
| * How to handle updates. The default/implicit behavior is REPLACE_BY_KEY | ||
| */ | ||
| defaultUpdateBehavior: optional enum UpdateBehavior { | ||
| REPLACE_BY_KEY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you plan to support delete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can do it here at the model level, since all AssetLabel instances will have to have the same UpdateBehavior. Instead, I'm thinking to use the IngestionMode field of IngestionParams to specify what kind of update is to be performed. So something like:
LabelService
-> update(new label)
-> call assetService.update(mode=LIVE) w/ default UpdateBehavior
-> delete(existing label)
-> call assetService.update(mode=DELETE_FROM_COLLECTION) which will override the UpdateBehavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even more generic than the LabelService, maybe we can have a CollectionsService just for handling Collections-type aspects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we do the collection, let's not have a separate service for it then. Probably you can explore the existing delete api we already have in MMG. And saying that we have default update behavior and default delete behavior etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I can also update the delete API to include IngestionParams
| /** | ||
| * Information about collections | ||
| */ | ||
| collection: optional CollectionAnnotation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have another annotation defined in the plugin, how will that work? can we consolidate the two here? I will prefer us to deprecate the ppl annotation here at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotation on the internal model will get translated to this pdl annotation (there will be logic in metadata-models-plugin to do this). We cannot fully deprecate the pdl annotation without either moving datahub-gma into a LI MP or starting to rely on proto in datahub-gma.
Essentially, the proto annotation (i.e. (.proto.mg.collection={...}) will get translated into this pdl annotation @gma.collection = {...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to process the annotation and do some customization on the annotation, can we just do it in MGA (i.e. a layer up of DAO). I think you will anyway need to check the update method and the collection type before we enter DAO layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the main tradeoff for where we put the annotation processing logic is how many db operations need to be done?
if we put in MGA, we need to do
- get() to get the latest value of the collection so we may manipulate it before calling dao.add
2a) getLatest() as part of the dao.add
2b) saveLatest()
if we put in DAO, we just don't need the first get() so it will just be 1 read, 1 write.
but agree with you that in the second case, we will have to process the annotation in both MGA and in DAO.
I admit that the performance gained by going with this approach is insignificant, let's go just put the annotation and processing in MGA instead. thanks for the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just saw this message, thanks for taking the suggestion. But for the part where we process the annotation on MGA, I would suggest you to call the add with preprocessFunction method to avoid the duplicate get call and make sure the update is atomic.
Summary
Add
@gma.collection.annotation for supporting collections. There are 3 items to specify:isCollection - should be true for collections. default false.
primaryKeys - struct (essentially a map) representing which fields to use as primary keys for collection-typed fields. for example, given:
I can specify that I want the primary key for this field to be the dataset_urn and actor fields. i.e.
"upstream": ["dataset_urn", "audit_stamp.actor"]Next step is to add logic to metadata models plugin to convert proto annotations to this pdl annotation
Testing Done
Add a unit test.
Checklist