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) Add deleteMany Dao method #496

Merged
merged 4 commits into from
Feb 10, 2025
Merged

(feat) Add deleteMany Dao method #496

merged 4 commits into from
Feb 10, 2025

Conversation

jphui
Copy link
Contributor

@jphui jphui commented Feb 7, 2025

Summary

In order to support the new Delete Aspect API, a deleteMany() method (with default-parameter variants) was added.

DeleteMany will be used to support the following spec:

/** Delete one or more Aspects for an URN */ 
rpc deleteAspects(DeleteAspectFromASSETRequest) returns (DeleteAspectFromASSETResponse) { option (.si.methodType) = DELETE; }

message DeleteAspectFromASSETRequest { 
/** Request context */ 
si.RequestContext context = 1; 
/** The URN of the asset for which aspects are being deleted. */ 
URN urn = 2; 
/** The list of aspect names to delete. */ 
repeated string aspectNames = 3; }

Testing Done

Adjusted additional unit testing and added a new one.

Checklist

Comment on lines +862 to +866
// entire delete operation should be atomic
runInTransactionWithRetry(() -> {
aspectClasses.forEach(x -> delete(urn, x, auditStamp, maxTransactionRetry, trackingContext));
return null;
}, maxTransactionRetry);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open Question 1

This implementation propagates maxTransactionRetry to the individual deletion level.

Alternatively, the "maxTransactionRetry" can be only a top-level retry setting that only applies to the overall transaction and NOT the individuals.

@Nonnull AuditStamp auditStamp,
int maxTransactionRetry,
@Nullable IngestionTrackingContext trackingContext) {
// entire delete operation should be atomic
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 was decided in the Spec Design Review as a comment from Yang.

@@ -811,6 +811,61 @@ public <ASPECT extends RecordTemplate> ASPECT add(@Nonnull URN urn, AspectUpdate
: unwrapAddResult(urn, result, auditStamp, trackingContext));
}

/**
* Deletes the latest version of aspects for an entity. Aspects must be a supported aspect types in {@code ASPECT_UNION}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validating whether an Aspect is deletable will be done "above" the datahub-gma level. This is decided here: https://docs.google.com/document/d/1aYq106Um6B3_AUKdR4QtowANO27C2GlChROdZj4vdlw/edit?disco=AAABcVgHQSg

* @param maxTransactionRetry maximum number of transaction retries before throwing an exception
*/
public void deleteMany(@Nonnull URN urn,
@Nonnull Set<Class<? extends RecordTemplate>> aspectClasses,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open Question 2

Use a Set as input to handle the case of duplicate Aspect classes for deletion.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 66.05%. Comparing base (707986e) to head (afb869d).

Files with missing lines Patch % Lines
...n/java/com/linkedin/metadata/dao/BaseLocalDAO.java 0.00% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #496      +/-   ##
============================================
- Coverage     66.16%   66.05%   -0.12%     
  Complexity     1536     1536              
============================================
  Files           137      137              
  Lines          5917     5927      +10     
  Branches        656      656              
============================================
  Hits           3915     3915              
- Misses         1723     1733      +10     
  Partials        279      279              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@yihzhou yihzhou left a comment

Choose a reason for hiding this comment

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

LGTM

@jphui jphui merged commit 2de35c0 into master Feb 10, 2025
2 checks passed
@jphui jphui mentioned this pull request Feb 12, 2025
3 tasks
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