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.

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

feat: add primitive type based Read Dao interface #319

Closed
wants to merge 1 commit into from

Conversation

JiaoMaWHU
Copy link
Contributor

@JiaoMaWHU JiaoMaWHU commented Nov 20, 2023

Summary

  • this pr attempts to add a primitive type based ReadDao interface to datahub-gma. The proposed interface removes the entity type requirement from original dao implementation, so that one instance of Dao can support multiple entities.

Approach Trade-Off

Walk through possible approaches where one generic API that can support multiple metadata entities.

Part A. Rest.li model

  1. create a union type of all entity snapshot. use this type in rest.li model.
    • pros: type safe server/client communication
    • cons: complicated schema, essentially the same as the legacy MXEv4, which is a group of aspects of multiple entities
  2. (preferred) use primitive types in rest.li model.
    • pros: simplified model schema
    • cons:
      • clients need to take care of (de)serialization of metadata models and pass it to GMS
      • GMS will need to deserialize primitive type input again and do validation (e.g., valid entity, aspect) in order to prevent bad and unregistered data
      • one should not develop business logic on top of this model, since it would require casting primitive types again to pdl record type.

Part B. Dao

  1. reuse current Dao. we register all entity Dao in one GMS based on current implementation, we route a request to its corresponding Dao.
    • pros: low LoE
    • cons: manual maintenance required for the Dao registration; not scalable
  2. remove the generic entity type T from Dao level to method level, so that we don't need to register multiple Dao.
    • pros: no maintenance cost; can leverage the advantages of strong type
    • cons: just like the current approach, Dao needs to be aware of domain logic, for example, it needs to understand what's a dataset snapshot if it's passed as T by clients; therefore not well-decoupled
  3. (preferred) create new Dao using primitive types.
    • pros: decouple Dao with domain logic, Dao will only need to take care of IO; no maintenance cost
    • cons: will need to implement light weight input validation to filter bad data

Conclusion

My preference is A.2 and B.3. A.1 is a legacy concept we moved away from. I prefer B.3 since it has a clear decouple between dao and API layer. Through the offline discussion with @yangyangv2, I want to acknowledge that we need to have good input validation and a base client library that can ease (de)serialization for users.

Following two sequence diagram demonstrate the flow.

Read sequence diagram. Following prs will implement this flow.

  • urn, aspect (FQCN), metadata are string type
    Generic Read   Write API - Read

Write sequence diagram. Write will not be prioritized for now but listed to make sure it's future-proof.

  • urn, aspect (FQCN), metadata are string type
    Generic Read   Write API - Write

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

@JiaoMaWHU JiaoMaWHU self-assigned this Nov 20, 2023
@JiaoMaWHU JiaoMaWHU changed the title feat: add generic read dao interface feat: add primitive type based Read Dao interface Nov 20, 2023
@JiaoMaWHU
Copy link
Contributor Author

close this pr as primitive type based read dao does not make sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant