Updating method for create(), update() and delete() to handle soft delete by setting deleted_ts by kotkar-pallavi · Pull Request #549 · linkedin/datahub-gma

kotkar-pallavi · 2025-06-19T02:10:15Z

This is PR#3 in a series of PRs which will contain code changes to support usage of deleted_ts column in DAO methods for read and write paths.
Prev:
PR#1 - #539
PR#2 - #544

Summary

Updated create implementation
- Update the SQL for create to follow a pattern like so:

INSERT INTO metadata_entity_foo (urn, a_urn, lastmodifiedon, lastmodifiedby,a_aspectfoo) 
VALUES (:urn, :a_urn, :lastmodifiedon, :lastmodifiedby,:aspect0) 
ON DUPLICATE KEY UPDATE a_aspectfoo= :aspect0,  deleted_ts = IF(deleted_ts IS NULL, CAST('DuplicateKeyException' AS UNSIGNED), deleted_ts);

Updated update implementation
- If this asset was previously marked for deletion and still not purged, updating the new aspect values and update delete_ts will be set to NULL. ref: updated SQL
Updated delete implementation
- Instead of doing a hard delete, deleting all aspects and then setting deleted_ts=NULL in same transaction

Testing Done

Added unit tests
Modified unit tests
./gradlew build

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Docs related to the changes have been added/updated (if applicable)

…lete by setting deleted_ts

kotkar-pallavi · 2025-06-19T02:11:53Z

dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java

-   * @return a collection of the deleted aspects (their value before deletion), each wrapped in an instance of
-   * {@link ASPECT_UNION}
-   */
-  protected Collection<ASPECT_UNION> deleteCommon(@Nonnull URN urn,


deleting this, because reusing the delete() implementation for deleting all aspects (this)
instead of doing Hard Asset deletion (deleting entire row)

kotkar-pallavi · 2025-06-19T02:13:43Z

dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java

      // Given: changeLog is disabled
      assertFalse(_enableChangeLog);
-      // When: updateWithOptimisticLocking is called
-      try {


removed these tests for New Schema, since now we will overwrite the aspect value during create even if it was marked as soft deleted using $gma_deleted.

ref: https://github.com/linkedin/datahub-gma/pull/549/files#diff-56020705a401d66f63fe0a2b1afd740f431e55a0ecc2a08fbb4c1ed65da3f559R312

jphui

Reviewed about half the files, super busy today, will take a look at the rest in a sec

If this asset was previously marked for deletion and still not purged, updating the new aspect values and update delete_ts will be set to NULL. ref: updated SQL

Does this mean that "if this record was deleted but not removed from the db ("not purged") and we do update(), then mark the record as not deleted"?

Does this go through the upsert check as well?

jphui · 2025-06-20T20:52:50Z

dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java

+          } catch (NullPointerException e) {
+            log.warn("Aspect {} for urn {} does not exist", aspectClass.getName(), urn);
+          }


Will this throw on both:

aspect class is not a supported aspect of the asset type?

aspect class is empty in the db?

i think aspect class not supported aspect of the asset type will throw a ModelValidationException from here: SQLSchemaUtils.java#L225

it will handle aspect class empty in db.
this scenario is possible for delete asset - user does not specify any aspect names, at service level we populate all aspect class names for delete call.

jphui · 2025-06-20T21:14:42Z

dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java

+    results.forEach((key, value) -> {
+      // Check if aspect value present to avoid null pointer exception
+      DeleteResult deleteResult = new DeleteResult(value, key);
+      deletedAspects.add(unwrapDeleteResult(urn, deleteResult, auditStamp, trackingContext, ChangeType.DELETE_ALL));


Few questions / checks:

seems there's a TODO related to this: https://jarvis.corp.linkedin.com/codesearch/result/?name=KafkaMetadataEventProducerUtil.java&path=metadata-models%2Fmetadata-dao-impl%2Fkafka-producer%2Fsrc%2Fmain%2Fjava%2Fcom%2Flinkedin%2Fmetadata%2Fdao%2Fproducer&reponame=linkedin-multiproduct%2Fmetadata-models#99

are there any remaining AI's here that could affect this impl?

I notice the implementation for unwrapDeleteResult() is different for the emission: https://jarvis.corp.linkedin.com/codesearch/result/?name=BaseLocalDAO.java&path=datahub-gma%2Fdao-api%2Fsrc%2Fmain%2Fjava%2Fcom%2Flinkedin%2Fmetadata%2Fdao&reponame=linkedin%2Fdatahub-gma#819

is it OK there is no equality check?

I think that TODO is more for ChangeType.DELETE_ALL vs DELETE. This was related to previous Delete impl when we were doing a hard delete. I will remove that TODO, it is not relevant anymore.

We are using unwrapDeleteResult to send events to HS to delete entire document. So i think with the ChangeType.DELETE_ALL we do not need to compare old and new value. please let me know if I am missing something.

jphui · 2025-06-20T21:32:14Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalAccess.java

+      deletedTsCheckForCreate.append(" = :aspect").append(i);
+      if (i != classNames.size() - 1) {
+        deletedTsCheckForCreate.append(", ");
+      }
+    }


Is this for loop tested in unit testing at all? (is it easy to test?)

good callout. this code will be used when more than asset with more than 1 aspect is being in created.
Updated test testCreateAfterAssetMarkedDeleted to create asset with 2 aspects and verify both aspect values with get.

sample vale from unit test:

INSERT INTO metadata_entity_foo (urn, a_urn, lastmodifiedon, lastmodifiedby,a_aspectfoo, a_aspectbar) VALUES (:urn, :a_urn, :lastmodifiedon, :lastmodifiedby,:aspect0, :aspect1) ON DUPLICATE KEY UPDATE a_aspectfoo = :aspect0, a_aspectbar = :aspect1, deleted_ts = IF(deleted_ts IS NULL, CAST('DuplicateKeyException' AS UNSIGNED), NULL);

kotkar-pallavi · 2025-06-20T22:21:13Z

Reviewed about half the files, super busy today, will take a look at the rest in a sec

If this asset was previously marked for deletion and still not purged, updating the new aspect values and update delete_ts will be set to NULL. ref: updated SQL

Does this mean that "if this record was deleted but not removed from the db ("not purged") and we do update(), then mark the record as not deleted"?

Yes, deleted_ts will be set to NULL and only the aspect being updated/created will be updated(!) If there were other aspects previously deleted they will continue to be marked deleted.

Does this go through the upsert check as well?

@jphui do you mean the upsert check that is done at the service level based on annotations?

jphui · 2025-07-07T18:58:26Z

Does this go through the upsert check as well?
@jphui do you mean the upsert check that is done at the service level based on annotations?

Yeah more of a sanity check than anything but for cases where upsert is disabled (?) this soft-deletion impl will work properly?

jphui · 2025-07-07T19:06:37Z

dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java

+   * @return a map of the deleted aspects (their value before deletion), each wrapped in an instance of
+   * {@link ASPECT_UNION}
   */
-  protected abstract Map<Class<? extends RecordTemplate>, Optional<? extends RecordTemplate>> permanentDelete(@Nonnull URN urn,
-      @Nonnull Set<Class<? extends RecordTemplate>> aspectClasses, @Nullable AuditStamp auditStamp,
-      int maxTransactionRetry, @Nullable IngestionTrackingContext trackingContext, boolean isTestMode);
+  protected abstract int permanentDelete(@Nonnull URN urn, boolean isTestMode);


update documentation here

what's the reasoning behind changing the return type from object to int? what does this help achieve?

now, in permanent delete we will only be marking the asset as deleted by setting deleted_ts=NOW(). actual aspect level (sofl) deletion happens in BaseLocalDao.deleteAll() and the map of aspect objects will be returned from there.

here int is for retuning how many db rows were updated (techincally will olways be 1 since 1row=1asset record).

jphui · 2025-07-07T19:07:01Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java

+        // do a get to ensure the urn does not already exist
+        // if exists and deletedTs is null, then throw an exception
+        // if exists and deletedTs is not null, then update the deletedTs to null and create records


thanks for the inline doc! is this just explaining what the next line "create" will do?

yes, describing high level behavior for create.
i added some comments closer to impl in EBeanLocalAccess.create

jphui · 2025-07-07T19:16:25Z

dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/IEbeanLocalAccess.java

   * @param oldTimestamp             old time stamp for optimistic lock checking
   * @param ingestionTrackingContext the ingestionTrackingContext of the MCE responsible for calling this update
   * @param isTestMode               whether the test mode is enabled or not
+   * @param softDeleteOverwrite      whether to overwrite soft deleted aspects marked with $gma_deleted


What happens if this is set to false? What is the expected behavior at the db level?

Could you add some documentation here clarifying this?

thanks for pointing out. i've updated documentation.
i am using softDeleteOverwrite to main same old behavior for old schema. softDeleteOverwrite=false for old schema since we dont have deleted_ts on old schema.

all soft deletion related code is in new schema only since we have added the deleted_ts column only in new schema tables.

jphui · 2025-07-07T19:18:26Z