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

fix(graph-edge): fix graph edge delete exception #12025

Conversation

david-leifker
Copy link
Collaborator

@david-leifker david-leifker commented Dec 4, 2024

Fixing an exception that occurs when deleting graph edges when the MCL is missing the record template. Code updated to check the previous aspect and the current aspect during a delete.

Additionally, if both aspects are missing avoid an exception and log a warning.

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). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Dec 4, 2024
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Dec 4, 2024
@@ -190,7 +190,10 @@ private void handleDeleteChangeEvent(
urn.getEntityType(), event.getAspectName()));
}

RecordTemplate aspect = event.getRecordTemplate();
final RecordTemplate aspect =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why can we end up with a null aspect? Seems that some of the implementations from RecordTemplate can be either Nullable and Nonnull Just curious if might be just better to return an empty record template that handling a mix of Nullable and Nonnull that could end up in NPEs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The MCL usually contains both a current aspect and a previous aspect, the previous aspect is null when the aspect is being first created. For a delete, it doesn't make sense that the current aspect would be populated necessarily. I would expect a previous aspect unless we hit a third case which is a delete of the key aspect of an entity which should technically be added separately but is complete redundant with the entity urn.

In any case, we need to respect that the data model defined both current and previous as optional here and here. Changing the model is a backwards incompatible change.

@datahub-cyborg datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Dec 4, 2024
@david-leifker david-leifker force-pushed the fix-graph-delete-exception branch 3 times, most recently from 4e80d67 to 6e74538 Compare December 4, 2024 23:37
@datahub-cyborg datahub-cyborg bot added pending-submitter-merge and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Dec 5, 2024
@david-leifker david-leifker force-pushed the fix-graph-delete-exception branch 2 times, most recently from 9617d37 to 39be9f1 Compare December 5, 2024 02:02
@david-leifker david-leifker force-pushed the fix-graph-delete-exception branch from 39be9f1 to 09ba17e Compare December 5, 2024 11:59
@david-leifker david-leifker merged commit 3f3f777 into datahub-project:master Dec 5, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-submitter-merge product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants