-
Notifications
You must be signed in to change notification settings - Fork 58
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: Support deletion in EbeanGenericLocalDAO and BaseEntityAgnosticAspectResource #454
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #454 +/- ##
============================================
+ Coverage 67.52% 67.69% +0.16%
- Complexity 1581 1606 +25
============================================
Files 142 143 +1
Lines 6159 6200 +41
Branches 667 675 +8
============================================
+ Hits 4159 4197 +38
- Misses 1717 1719 +2
- Partials 283 284 +1 ☔ View full report in Codecov by Sentry. |
if (shouldSkipMAEUpdate(newValue)) { | ||
return null; | ||
} | ||
_producer.produceAspectSpecificMetadataAuditEvent(urn, null, newValue, auditStamp, trackingContext, ingestionMode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about
if (!shouldSkipMAEUpdate(newValue)) {
_producer.produceAspectSpecificMetadataAuditEvent(urn, null, newValue, auditStamp, trackingContext, ingestionMode);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg
if (shouldSkipMAEUpdate(newValue)) { | ||
return null; | ||
} | ||
_producer.produceAspectSpecificMetadataAuditEvent(urn, currentValue, newValue, auditStamp, trackingContext, ingestionMode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
if (!shouldSkipMAEUpdate(newValue)) {
_producer.produceAspectSpecificMetadataAuditEvent(urn, currentValue, newValue, auditStamp, trackingContext, ingestionMode);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sg
@@ -128,7 +152,7 @@ public Optional<GenericLocalDAO.MetadataWithExtraInfo> queryLatest(@Nonnull Urn | |||
final PrimaryKey key = new PrimaryKey(urn.toString(), aspectName, LATEST_VERSION); | |||
EbeanMetadataAspect metadata = _server.find(EbeanMetadataAspect.class, key); | |||
|
|||
if (metadata == null || metadata.getMetadata() == null) { | |||
if (metadata == null || metadata.getMetadata() == null || DELETED_VALUE.equals(metadata.getMetadata())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so just to confirm the metadata will be set to DELETED_VALUE in mysql db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please see
datahub-gma/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanGenericLocalDAO.java
Line 277 in 023f1bb
aspect.setMetadata(DELETED_VALUE); |
Summary
Support deletion in
EbeanGenericLocalDAO
andBaseEntityAgnosticAspectResource
We need to have generic deletion support for annotation metadata.
Long-term wise, team decided to have deletion support in every grpc endpoint.
However, to meet the timeline of the annotation use case, a
delete
method will be added in theBaseEntityAgnosticAspectResource
so thatannotationGms
resource can expose this action.Details
delete
method and not send MAE for updatesqueryLatest
andbackfill
methods to accommodate deleted dataTesting Done
Checklist