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(relationship 2.0): use REMOVE_ALL_EDGES_FROM_SOURCE by default for ingesting 2.0 relationships #463

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

jsdonn
Copy link
Contributor

@jsdonn jsdonn commented Oct 29, 2024

Summary

In Model 1.0, when new relationships are ingested, existing relationships are handled (i.e. removed) according to user-defined removal options. There are 4 possible options:

  1. REMOVE_NONE
  2. REMOVE_ALL_EDGES_FROM_SOURCE
  3. REMOVE_ALL_EDGES_TO_DESTINATION
  4. REMOVE_EDGES_FROM_SOURCE_TO_DESTINATION
  1. has no use cases and will no longer be supported.
  2. will be the default
  3. will no longer be supported. in 2.0, relationships must always be modeled such that the asset they are derived from is the source in the relationship.
  4. has use cases in AIM but moving forward, it will be up to the user to replicate these now "non-default" behaviors via pre-ingestion lambdas. Essentially, whatever the user provides to us in the aspect model, we will ingest using REMOVE_ALL_EDGES_FROM_SOURCE behavior.

Testing Done

adjusted some unit tests

Checklist

* @throws IllegalArgumentException if the asset urn and the relationship source urn don't match
*/
public static <URN, RELATIONSHIP extends RecordTemplate> void validateRelationshipSource(URN assetUrn, RELATIONSHIP relationship) {
Urn sourceUrn = getSourceUrnFromRelationship(relationship);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we still require the relationship to define the source?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. the source urn that will be used will be the asset urn during ingestion.

.map(relationships -> new BaseLocalRelationshipBuilder.LocalRelationshipUpdates(
relationships, relationships.get(0).getClass(), BaseGraphWriterDAO.RemovalOption.REMOVE_NONE))
.map(relationships -> new LocalRelationshipUpdates(
relationships, relationships.get(0).getClass(), BaseGraphWriterDAO.RemovalOption.REMOVE_ALL_EDGES_FROM_SOURCE))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this code have any impact to the existing V1 relationships, before they are fully migrated to V2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is all in the if block

if (_relationshipSource == RelationshipSource.ASPECT_METADATA)

so it will only affect the v2 models. @ybz1013 is working to make a "dual-write" mode which will write both v1 and v2 relationships (into different tables).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

He will also add a validation to make sure that the v1 relationships (derived from relationship builder) == v2 relationships (extracted from aspect) while in dual-write mode so that we can avoid data inconsistencies during the transition phase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks for confirming

Copy link
Contributor

@hongliu2024 hongliu2024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsdonn jsdonn merged commit a630243 into linkedin:master Nov 6, 2024
2 checks passed
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.

3 participants