feat(local relationship): support local relationship tables in old schema #391

jsdonn · 2024-07-24T22:33:53Z

Summary

As part of the graph query service, we want to support local relationship tables in all GMSes, regardless of which database schema mode they are on (i.e. NEW_SCHEMA_ONLY vs OLD_SCHEMA_ONLY). This requires adding support for local relationship ingestion and querying in the old schema.

Ingestion

Before: local relationship ingestion was bundled with new schema entity table ingestion in EbeanLocalAccess. While bundled in the same add method, there is no technical dependency between the two.

After: local relationship ingestion has been moved to EbeanLocalDAO::saveLatest. The logic is the same:

check if the incoming update is a soft deletion.
if soft deletion, check if the aspect to be soft-deleted has any relationships tied to it.
if yes, throw an error. No aspect soft deletion allowed if it is associated with local relationships.
ingest aspect as usual (into entity tables in new schema or metadata_aspect in old schema)
ingest local relationships using the same logic as before.

Query

Before: querying for local relationships using the findRelationships API would join multiple tables together (relationship + entity tables) to satisfy any filter conditions. This logic was built specifically for the new schema.

After: EbeanLocalRelationshipQueryDAO now has a schemaConfig similar to EbeanLocalDAO, which can take on the values of NEW_SCHEMA_ONLY, DUAL_SCHEMA, and OLD_SCHEMA_ONLY. It can be set using setSchemaConfig(). findRelationships will use different SQL queries depending on the value of schemaConfig. In new schema mode, everything is the same as before.

In old schema mode, there are certain restrictions. Only 1 filter is allowed per source or destination entity. That filter, if it exists, MUST be on the urn field (and not the aspect field for example). Meaning - no joins are necessary; all information in the allowed query can be found just by looking at the local relationship table. This restriction is in place because filtering on metadata_aspect is not feasible since the metadata column is not indexed. Thus, the resulting SQL will look something like:

SELECT rt.* FROM metadata_relationship_<relationship_type> rt
WHERE deleted_ts IS NULL
{ (optional) AND rt.source = urn:li:<sourceType>:1 }
{ (optional) AND rt.destination = urn:li:<destinationType>:2 }

Other Details

Introduced RelationshipSource enum which can have 2 values: RELATIONSHIP_BUILDERS and ASPECT_METADATA. This enum will be used to tell the DAO where to ingest relationship metadata from. Right now, all relationship data comes from looking up an Aspect class in a local relationship builder registry to retrieve the corresponding relationship builder. Then, the relationship is built using that relationship builder and ingested.

In the future, relationships will be stored directly in the aspect metadata so maintaining relationship builders and a registry will not be necessary. However, I will leave the implementation of supporting the ASPECT_METADATA enum for a later PR.

Testing Done

Updated/refactored some unit tests.

Checklist

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

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java

yangyangv2

minor comments, thanks for the hard / great work!

The PR description is very helpful!

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java

yangyangv2 · 2024-08-09T15:07:57Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java

-   * Set a local relationship builder registry.
+   * Provide a local relationship builder registry. Local relationships will be built based on the builders during data ingestion.
+   * @param localRelationshipBuilderRegistry All local relationship builders should be registered in this registry.
+   *                                         Can be set to null to turn off local relationship ingestion.


in which scenario we want to disable the local relationship? routing GMS?

this is little implicit operation control --- should we keep the registry nonNull but have an more explicit control to enable/disable local relationship ingestion?

We want to disable the local relationships when a relationship is not ready to be onboarded yet (like when the table has not been created yet).

In the future, we might not need this; we can just assume that all relationships will be ingested. But for now, while we still have ES graph and MySQL local relationship tables, we only want to ingest the relationships that are actually onboarded to MySQL local relationships.

makes sense, can you include a brief explanation in the comment?

yangyangv2 · 2024-08-09T15:11:28Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipQueryDAO.java

@@ -80,6 +87,9 @@ public EbeanLocalRelationshipQueryDAO(EbeanServer server) {
  @Nonnull
  public <SNAPSHOT extends RecordTemplate> List<SNAPSHOT> findEntities(@Nonnull Class<SNAPSHOT> snapshotClass,
      @Nonnull LocalRelationshipFilter filter, int offset, int count) {
+    if (_schemaConfig == EbeanLocalDAO.SchemaConfig.OLD_SCHEMA_ONLY) {
+      throw new UnsupportedOperationException("findEntities is not supported in OLD_SCHEMA_MODE");


should this be a Checked Exception to make sure the OLD_SCHEMA can properly handle it? or can we be sure OLD_SCHEMA will not run into this logic

Can you clarify what you mean by checked exception?

the caller needs to explicitly catch and handle the exception.

added a throws UnsupportedOperationException to the method signature

UnsupportedOperationException is a "RuntimeException" and it does not make caller to explicitly catch it. You can either switch to some exception that extends Exception instead of RuntimeException.

My intention was a question more making the client to be cautious when using this findEntity methods as they may be running on old schema, otherwise they'll see this issue at runtime.. But if you think this risk is low, then you can stay with the Runtime Exception, but make the documentation clear.

Understood. Changed to OperationNotSupportedException from javax which extends Exception. Also added to the javadocs.

In general, I don't expect many non-MG users to use the localrelationship dao. As far as I know only the lineage accessor in TMS uses it and we have control over that.

yangyangv2 · 2024-08-09T15:12:02Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalRelationshipQueryDAO.java

@@ -104,6 +114,9 @@ public <SRC_SNAPSHOT extends RecordTemplate, DEST_SNAPSHOT extends RecordTemplat
      @Nonnull Class<DEST_SNAPSHOT> destinationEntityClass, @Nonnull LocalRelationshipFilter destinationEntityFilter,
      @Nonnull Class<RELATIONSHIP> relationshipType, @Nonnull LocalRelationshipFilter relationshipFilter, int minHops,
      int maxHops, int offset, int count) {
+    if (_schemaConfig == EbeanLocalDAO.SchemaConfig.OLD_SCHEMA_ONLY) {
+      throw new UnsupportedOperationException("findEntities is not supported in OLD_SCHEMA_MODE");


same as line 91

added a throws UnsupportedOperationException to the method signature

Changed to OperationNotSupportedException from javax which extends Exception. Also added to the javadocs.

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SQLStatementUtils.java

…hema

yangyangv2

LGTM, thanks for addressing the comments.

yangyangv2 reviewed Aug 9, 2024

View reviewed changes

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java Show resolved Hide resolved

yangyangv2 reviewed Aug 9, 2024

View reviewed changes

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java Outdated Show resolved Hide resolved

yangyangv2 reviewed Aug 9, 2024

View reviewed changes

Justin Donn added 4 commits August 13, 2024 15:33

feat(local relationship): support local relationship tables in old sc…

88a23d3

…hema

fix unit tests

8a04149

minor changes; address first round of comments

1eb5d79

rebase

124dfad

jsdonn force-pushed the old_schema_local_relationship branch from 95b02b4 to 124dfad Compare August 13, 2024 23:01

Justin Donn added 3 commits August 13, 2024 16:04

delete an unused var

2adf3f0

address comments - add some better javadocs

211422a

add javadocs to findEntities

d3e5146

yangyangv2 approved these changes Aug 14, 2024

View reviewed changes

fix unit tests

e8fe3f7

ybz1013 merged commit be56ee0 into linkedin:master Aug 15, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(local relationship): support local relationship tables in old schema #391

feat(local relationship): support local relationship tables in old schema #391

jsdonn commented Jul 24, 2024 •

edited

Loading

yangyangv2 left a comment •

edited

Loading

yangyangv2 Aug 9, 2024

jsdonn Aug 13, 2024

yangyangv2 Aug 14, 2024

yangyangv2 Aug 14, 2024

yangyangv2 Aug 9, 2024

jsdonn Aug 13, 2024

yangyangv2 Aug 14, 2024

jsdonn Aug 14, 2024

yangyangv2 Aug 14, 2024

jsdonn Aug 14, 2024

jsdonn Aug 14, 2024

yangyangv2 Aug 9, 2024

jsdonn Aug 14, 2024

jsdonn Aug 14, 2024

yangyangv2 left a comment

feat(local relationship): support local relationship tables in old schema #391

feat(local relationship): support local relationship tables in old schema #391

Conversation

jsdonn commented Jul 24, 2024 • edited Loading

Summary

Ingestion

Query

Other Details

Testing Done

Checklist

yangyangv2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangyangv2 left a comment

Choose a reason for hiding this comment

jsdonn commented Jul 24, 2024 •

edited

Loading

yangyangv2 left a comment •

edited

Loading