Persist emitTime from IngestionTrackingContext to the new entity tables and use emitTime during backfill #329

derekpham · 2023-12-01T20:22:07Z

Context

This is continuation of #328
This PR makes 2 changes:

Persist emitTime to the new entity table if IngestionTrackingContext is provided
Use emitTime when checking whether we should backfill the entity
If oldEmitTime is available and smaller than new event's emit time -> backfill
If oldEmitTime is not available and last modified time smaller than new event's emit time -> backfill

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)

…e emitTime field when during backfill logic if available

jsdonn

Overall lgtm, small comments

jsdonn · 2023-12-04T23:26:15Z

dao-api/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl

+   *
+   * This will be null in the following scenarios:
+   * - The record is still on the old schema
+   * - The record inserted before we started persisting emitTime to the new schema


"was still on the old schema"?
or "record was inserted"

the latter makes more sense

jsdonn · 2023-12-04T23:30:47Z

dao-api/src/main/pegasus/com/linkedin/metadata/query/ListResultMetadata.pdl

+       *
+       * This will be null in the following scenarios:
+       * - The record is still on the old schema
+       * - The record inserted before we started persisting emitTime to the new schema


so I guess we won't backfill these values and just fall back on the naive approach if this val doesn't exist?

yeah that's the plan since most rows will be missing emitTime anyway. Based on the last discussion, we'll use lastmodifiedtime in place of emitTime for comparison.

jsdonn · 2023-12-04T23:32:15Z

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

+      @Nonnull Class<ASPECT> aspectClass,
+      @Nonnull AuditStamp auditStamp,
+      @Nullable Timestamp oldTimestamp,
+      @Nullable IngestionTrackingContext ingestionTrackingContext) {


messageId was never used before?

nope. Intellij didn't have much trouble removing that param.

does it mean we'll have a new messageId during the backfill? or the backfill's messageId is always null?

the param was before I added the backfill feature. It was an unused param.

talked offline with @yangyangv2 . For the method add, I will switch param messageId with the ingestionTrackingContext

derekpham · 2023-12-04T23:51:43Z

@jsdonn should we also reset isBackfill back to false when emitting MAE?

derekpham · 2023-12-05T00:53:30Z

@jsdonn here's what i meant https://github.com/derekpham/datahub-gma/blob/1bf3ea702e148e9f4bf9f5f605d622ab117ca678/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java#L580

jsdonn · 2023-12-05T19:30:32Z

@derekpham oh yeah that's a good idea. Even though it should be default to false (right?), definitely wouldn't hurt to add it explicitly there during regular MAE emission

derekpham · 2023-12-05T19:42:38Z

@jsdonn It's default is false but for backfill MCE we should explicitly set it back to false?

yangyangv2 · 2023-12-05T21:09:34Z

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

@@ -555,6 +559,10 @@ private <ASPECT extends RecordTemplate> ASPECT_UNION unwrapAddResultToUnion(URN

  private <ASPECT extends RecordTemplate> ASPECT unwrapAddResult(URN urn, AddResult<ASPECT> result, @Nonnull AuditStamp auditStamp,
      @Nullable IngestionTrackingContext trackingContext) {
+    if (trackingContext != null) {
+      trackingContext.setBackfill(false); // reset backfill since MAE won't be a backfill event


in what scenario trackingContext will be null and we want to enforce it's setBackfill to be false?

Can we log such as enforcement activity to capture unexpected cases?

it's null when someone calls ingest instead of ingestWithTracking. I can't think of a case where we don't want to set backfill to false.

^^ this isBackfill field in the ingestionTrackingContext is ONLY used for MCE backfills right? I believe Jinxin added a separate field for MAE backfill. If that's the case then yeah we always want to set it to false during MAE emission. But I hope that this is clear enough for ourselves and future devs...might be a little confusing to have 2 backfill related fields in an MAE.

yangyangv2 · 2023-12-05T21:20:49Z

dao-api/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl

@@ -37,6 +37,11 @@ record AuditedAspect {
   * This value is different from lastmodifiedon / the timestamp in AuditStamp since auditStamp
   * is created when the restli resource receives the ingestion request.
   * This is set by the MCE producers (or MCE consumers if not set by producers)
+   *
+   * This will be null in the following scenarios:
+   * - The record is still on the old schema


nit: do you mean "The record is from the old schema"? to how understand "still on"?

yangyangv2 · 2023-12-05T21:24:29Z

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

+      @Nonnull Class<ASPECT> aspectClass,
+      @Nonnull AuditStamp auditStamp,
+      @Nullable Timestamp oldTimestamp,
+      @Nullable IngestionTrackingContext ingestionTrackingContext) {


does it mean we'll have a new messageId during the backfill? or the backfill's messageId is always null?

yangyangv2 · 2023-12-05T21:32:09Z

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

@@ -151,6 +156,9 @@ public <ASPECT extends RecordTemplate> int addWithOptimisticLocking(@Nonnull URN
        .setLastmodifiedby(actor)
        .setLastmodifiedon(new Timestamp(timestamp).toString())
        .setCreatedfor(impersonator, SetMode.IGNORE_NULL);
+    if (ingestionTrackingContext != null) {


it would be safer and more clear to still create IngestionTrackingContext in the input and here we check

} ```

I think your comment is missing some code?

yangyangv2 · 2023-12-05T21:36:19Z

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

-                  && trackingContext.getEmitTime() > oldAuditStamp.getTime());
-
-      log.info("Encounter backfill event. Tracking context: {}. Urn: {}. Aspect class: {}. Old audit stamp: {}. "
+              // ingestionTrackingContext if not null should always have emitTime. If emitTime doesn't exist within


Is this the expectation in IngestionTrackingContext? the model definition is optional for emitTime.

Can we make the logic/expectation to be consistent with the model definition?

updated the comment

jsdonn

LGTM overall. One comment on having 2 backfill-related fields in the MAE but I guess we're kinda past the modeling phase.

Please get @yangyangv2 's approval for ship-it.

yangyangv2

last minor comment. The rest LGTM. Thanks for addressing all the comments!

yangyangv2 · 2023-12-12T13:15:13Z

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

@@ -93,8 +92,8 @@ public void ensureSchemaUpToDate() {
  @Override
  @Transactional
  public <ASPECT extends RecordTemplate> int add(@Nonnull URN urn, @Nullable ASPECT newValue, @Nonnull Class<ASPECT> aspectClass,
-      @Nonnull AuditStamp auditStamp, @Nullable UUID messageId) {
-    return addWithOptimisticLocking(urn, newValue, aspectClass, auditStamp, null, null);
+      @Nonnull AuditStamp auditStamp, IngestionTrackingContext ingestionTrackingContext) {


Please put a @nullable tag on ingestionTrackingContext.

yangyangv2

LGTM, thanks!

feat(backfill): Persist emitTime for the new schema as well as use th…

1bf3ea7

…e emitTime field when during backfill logic if available

jsdonn reviewed Dec 4, 2023

View reviewed changes

update docs

4c5926f

Reset backfill to false when emitting MAE

dcf30a1

yangyangv2 reviewed Dec 5, 2023

View reviewed changes

Derek Pham added 2 commits December 6, 2023 21:34

Address comments

2080708

Comment

f64f476

jsdonn reviewed Dec 7, 2023

View reviewed changes

yangyangv2 reviewed Dec 12, 2023

View reviewed changes

Add nullable annotation

3c3adb6

yangyangv2 approved these changes Dec 12, 2023

View reviewed changes

yangyangv2 merged commit 5874c4f into linkedin:master Dec 12, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist emitTime from IngestionTrackingContext to the new entity tables and use emitTime during backfill #329

Persist emitTime from IngestionTrackingContext to the new entity tables and use emitTime during backfill #329

derekpham commented Dec 1, 2023

jsdonn left a comment

jsdonn Dec 4, 2023

derekpham Dec 4, 2023

derekpham Dec 4, 2023

jsdonn Dec 5, 2023

derekpham Dec 5, 2023

jsdonn Dec 4, 2023

derekpham Dec 4, 2023

jsdonn Dec 4, 2023

derekpham Dec 4, 2023

yangyangv2 Dec 5, 2023

derekpham Dec 5, 2023

derekpham Dec 7, 2023

derekpham commented Dec 4, 2023

derekpham commented Dec 5, 2023

jsdonn commented Dec 5, 2023

derekpham commented Dec 5, 2023

yangyangv2 Dec 5, 2023

derekpham Dec 5, 2023

jsdonn Dec 7, 2023

yangyangv2 Dec 5, 2023

yangyangv2 Dec 5, 2023

yangyangv2 Dec 5, 2023

derekpham Dec 5, 2023

yangyangv2 Dec 5, 2023

derekpham Dec 7, 2023

jsdonn left a comment

yangyangv2 left a comment

yangyangv2 Dec 12, 2023

derekpham Dec 12, 2023

yangyangv2 left a comment

Persist emitTime from IngestionTrackingContext to the new entity tables and use emitTime during backfill #329

Persist emitTime from IngestionTrackingContext to the new entity tables and use emitTime during backfill #329

Conversation

derekpham commented Dec 1, 2023

Context

Checklist

jsdonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpham commented Dec 4, 2023

derekpham commented Dec 5, 2023

jsdonn commented Dec 5, 2023

derekpham commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdonn left a comment

Choose a reason for hiding this comment

yangyangv2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangyangv2 left a comment

Choose a reason for hiding this comment