fix(relationship 2.0): add aspect column to relationship tables and fix soft deletion #479
Conversation
dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SQLStatementUtils.java
Show resolved
Hide resolved
| deletionSQL.setParameter(CommonColumnName.SOURCE, source.toString()); | ||
| if (_useAspectColumnForRelationshipRemoval) { | ||
| deletionSQL.setParameter(CommonColumnName.ASPECT, aspectClass.getCanonicalName()); |
There was a problem hiding this comment.
if _useAspectColumnForRelationshipRemoval is default to false, wouldn't that cause the sql update here to have 2 less cols?
In what case will we use the default false value?
There was a problem hiding this comment.
or I guess this is a GMS level config, and once its set to true it will always be true.
There was a problem hiding this comment.
Can you clarify what you mean by 2 less cols? It should just have 1 less col aspect right?
| ASPECT_METADATA, // Look at the aspect model and extract any relationship from its fields | ||
| RELATIONSHIP_BUILDERS // Use the relationship registry, which relies on relationship builders | ||
| } | ||
| private boolean _useAspectColumnForRelationshipRemoval = false; |
There was a problem hiding this comment.
I guess this will make my dual write PR obsolete. I will close that.
There was a problem hiding this comment.
Oh yeah, I guess we are not doing that anymore. But it's not wasted work, eventually if we need to migrate v1 to v2 we will need it again and we can reference the old PR.
dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java
Show resolved
Hide resolved
| List<RELATIONSHIP> relationships = new ArrayList<>(); | ||
| localRelationshipUpdates.forEach(localRelationshipUpdate -> relationships.addAll(localRelationshipUpdate.getRelationships())); | ||
| _localRelationshipWriterDAO.removeRelationships(urn, aspectClass, relationships); | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
From the comments
- @return List of LocalRelationshipUpdates that were executed
should the return be a list of removed relationships?
There was a problem hiding this comment.
Updated to clarify - it will return nothing when soft-deleting only
| String insertTemplate = "INSERT INTO %s (metadata, source, source_type, destination, destination_type, lastmodifiedon, lastmodifiedby)" | ||
| + " VALUES ('{\"metadata\": true}', '%s', '%s', '%s', '%s', '1970-01-01 00:00:01', 'unknown')"; | ||
| if (_useAspectColumnForRelationshipRemoval) { |
There was a problem hiding this comment.
what is the migration plan for making all the relationship tables to include the aspect column? the shorter we finish this migration, the better. And the new relationships should by default to include the aspect column
There was a problem hiding this comment.
current local relationship cases are AIM tables and downstreamof.
For AIM, we have to wait until they deprecate their current local relationship tables and onboard new ones.
For downstreamof, it should be relatively straightforward to add the new column, index, and set all aspect values to UpstreamLineage.
ALTER TABLE metadata_relationship_downstreamof ADD COLUMN aspect VARCHAR(300);
CREATE INDEX idx_aspect ON metadata_relationship_downstreamof (aspect);
UPDATE metadata_relationship_downstreamof SET aspect = 'com.linkedin.common.UpstreamLineage';
Summary
Add
aspectcolumn to relationship tables so that during relationship deletion, only the relationships derived from the same aspect that is being ingested can be deleted. This is to prevent accidental deletion of relationships which can be derived from multiple aspects.Added a flag
useAspectColumnForRelationshipRemovalwhich defaults to false to be backwards compatible.Updated relationship soft deletion so it always works now whenever an aspect is soft-deleted.
Did some code cleanup of the RelationshipSource enum which is no longer needed since it will always try to get relationships from relationship builders first before looking in the aspect.
Testing Done
add unit tests
Checklist