feat(retention): Add retention to Local DB #3715

dexter-mh-lee · 2021-12-10T14:22:04Z

Add retention to Local DB. To do this:

Added RetentionService that is coupled with EntityService

Manages retention policies (get, set, delete policies for entity, aspect combo - both optional to set defaults)
Apply retention policy to a given urn, aspect
Apply retention policies in batch

Created a DataHubRetention entity to store the retention policies. This lets us use all the useful capabilities that an entity has within entity service
Setup triggering logic for applying retention.
a) Each call to setRetention triggers a batchApplyRetention for the affected rows
a) bootstrap step: on start-up, if there are no retention ingested, it sets retention for default based on env variable, which inturn calls batchApplyRetention for all rows

Checklist

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

github-actions · 2021-12-10T14:48:13Z

Unit Test Results

    45 files +  1     45 suites +1 43m 40s ⏱️ + 5m 46s
  635 tests +12   577 ✔️ +  6 58 💤 +6 0 ❌ ±0
1 419 runs +32 1 345 ✔️ +26 74 💤 +6 0 ❌ ±0

Results for commit fb83735. ± Comparison against base commit 36cf835.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2021-12-10T17:20:41Z

docs/advanced/db-retention.md

@@ -0,0 +1,79 @@
+# Configuring Database Retention
+
+## Goal


jjoyce0510 · 2021-12-10T17:21:33Z

metadata-io/src/main/java/com/linkedin/metadata/entity/EntityService.java

-  private Boolean _emitAspectSpecificAuditEvent = false;
+  @Getter
+  @Setter
+  private RetentionService retentionService;


This should never be null, right?

jjoyce0510 · 2021-12-10T17:36:53Z

metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanEntityService.java

-
-  @Nonnull
-  private UpdateAspectResult ingestAspectToLocalDB(@Nonnull final Urn urn, @Nonnull final String aspectName,
+  protected UpdateAspectResult ingestAspectToLocalDB(@Nonnull final Urn urn, @Nonnull final String aspectName,


Glad we are factoring this out.

jjoyce0510 · 2021-12-10T19:08:43Z

metadata-io/src/test/java/com/linkedin/metadata/entity/EbeanEntityServiceTest.java

+
+    // Reset retention policies
+    _retentionService.setRetention(null, null, new DataHubRetentionInfo().setRetentionPolicies(new RetentionArray(
+        ImmutableList.of(new Retention().setVersion(new VersionBasedRetention().setMaxVersions(1))))));


What happens if max version is 10?

It will just not do anything. It doesn't revive deleted aspects!

but if you ingest more after setting it to 10, it will ingest without deleting until it reaches 10 versions

jjoyce0510 · 2021-12-10T19:09:19Z

...s/src/main/java/com/linkedin/metadata/boot/factories/IngestRetentionPoliciesStepFactory.java

+  @Qualifier("retentionService")
+  private RetentionService _retentionService;
+
+  @Value("${entityService.retention.disable}")


the other configs use "enabled" as the flags. can we also do that here to keep things consistent?

ah makes sense. on it

jjoyce0510 · 2021-12-10T22:59:44Z

...ce/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestRetentionPoliciesStep.java

+    // 0. Execute preflight check to see whether we need to ingest policies
+    log.info("Ingesting default retention...");
+
+    // Whether we are at clean boot or not.


does this check if we are at clean boot or if retention has been disabled by a flag?

need to update comment

jjoyce0510 · 2021-12-10T23:01:47Z

...ce/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestRetentionPoliciesStep.java

+
+    // 4. Set the specified retention policies
+    log.info("Setting {} policies", retentionPolicyMap.size());
+    boolean hasUpdate = false;


why not just shortcircuit and call _retentionservice.batcha... inside the if directly?

ah because, I want to batch apply after setting all retention policies. Each set retention sets a single policy and returns whether that set caused an actual update. If there is at least one update, we apply retention to all

jjoyce0510 · 2021-12-10T23:02:53Z

...ce/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestRetentionPoliciesStep.java

+    return parseYamlRetentionConfig(retentionFileOrDir);
+  }
+
+  private Map<DataHubRetentionKey, DataHubRetentionInfo> parseYamlRetentionConfig(File retentionConfigFile)


this is great ! is there a comment about what the expected structure of a given yaml file should be?

jjoyce0510 · 2021-12-10T23:03:46Z

metadata-service/factories/src/main/resources/application.yml

+    entityRegistry:
+      path: ${ENTITY_REGISTRY_PLUGIN_PATH:/etc/datahub/plugins/models}
+    retention:
+      path: ${RETENTION_PLUGIN_PATH:/etc/datahub/plugins/retention}


jjoyce0510 · 2021-12-10T23:04:00Z

metadata-service/factories/src/main/resources/application.yml

+
+entityService:
+  retention:
+    disable: ${ENTITY_SERVICE_DISABLE_RETENTION:true}


same as above -- enabled / enable

jjoyce0510 · 2021-12-10T23:04:18Z

metadata-service/factories/src/main/resources/application.yml


 pluginEntityRegistry:
-  path: ${ENTITY_REGISTRY_PLUGIN_PATH:$HOME/.datahub/plugins/models}
+  path: ${ENTITY_REGISTRY_PLUGIN_PATH:/etc/datahub/plugins/models}


is this still necessary?

oops. removing

jjoyce0510 · 2021-12-10T23:05:12Z

metadata-service/war/src/main/resources/boot/retention.yaml

@@ -0,0 +1,14 @@
+- entity: "*"
+  aspect: "*"
+  retention:


minor nit: do we need the extra "retention" field?

could it be

entity
aspect
retentionPollicies

yeah. that's ideal, but I wanted to simply convert the section into DataHubRetentionInfo object without adding more logic there, which requires this field

jjoyce0510 · 2021-12-10T23:05:39Z

metadata-utils/src/main/java/com/linkedin/metadata/utils/EntityKeyUtils.java

    for (RecordDataSchema.Field field : keyAspect.schema().getFields()) {
      Object value = keyAspect.data().get(field.getName());
-      String valueString = value.toString();
+      String valueString = value == null ? "" : value.toString();


jjoyce0510 · 2021-12-10T23:08:43Z

metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanRetentionService.java

+        .or();
+    boolean retentionApplied = false;
+    for (Retention retention : retentionPolicies) {
+      if (retention.hasVersion() && applyVersionBasedRetention(urn, aspectName, deleteQuery, retention.getVersion(),


This code initially confused me because I didn't realize that deleteQuery was being mutated inside of applyVersionBasedRetention as "querySoFar"... what if that method simply returned a query and we inserted it into the expression list here?

That was what I wanted to do, but didn't think there was a way. Looking again, there might be a way. let me see

jjoyce0510 · 2021-12-10T23:12:08Z

metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanRetentionService.java

+    return _server.find(EbeanAspectV2.class)
+        .select(String.format("%s, %s, %s", EbeanAspectV2.URN_COLUMN, EbeanAspectV2.ASPECT_COLUMN,
+            EbeanAspectV2.METADATA_COLUMN))
+        .where()


hopefully this query will not be super expensive. i was planning to migrate policies away from using a query like this to using elastic directly.. i guess if its once a boot up maybe not too bad

better than for policies

Right. From tracing, I saw that these queries are sub-10ms queries, since we start with urn, aspect.

dexter-mh-lee · 2021-12-10T23:16:32Z

metadata-io/src/main/java/com/linkedin/metadata/entity/RetentionService.java

+  private void validateRetentionInfo(DataHubRetentionInfo retentionInfo) {
+    Set<String> retentionsSoFar = new HashSet<>();
+    for (Retention retention : retentionInfo.getRetentionPolicies()) {
+      if (retention.data().size() != 1) {


yup so we support it as list of retention. Inside a single Retention object, we should have just one set, kind of emulating a union behavior. We could instead have a single Retention object instead of a list of retention objects as policy and set as many policies as needed. WDYT?

jjoyce0510 · 2021-12-10T23:18:38Z

metadata-models/src/main/pegasus/com/linkedin/retention/Retention.pdl

+ * Base class that encapsulates different retention policies.
+ * Only one of the fields should be set
+ */
+record Retention {


the other option is to have a type and arguments:

type: enum Retentiontype: {
indefinite,
version,
time
}
// set if type is version
maxVersion: optional long

// set if type is time
maxAgeInseconds

this structure just makes reading the object in code a bit more complex.. is there a reason you went with the approach of just setting a field and letting the reader check the presence of each?

Hmn since there is no shared fields, I think this approach is much easier to read. Otherwise, each type needs to be mapped to the field that needs to be filled out in code instead of in pdl.

jjoyce0510 · 2021-12-10T23:19:44Z

metadata-io/src/main/java/com/linkedin/metadata/entity/RetentionService.java

+  private void validateRetentionInfo(DataHubRetentionInfo retentionInfo) {
+    Set<String> retentionsSoFar = new HashSet<>();
+    for (Retention retention : retentionInfo.getRetentionPolicies()) {
+      if (retention.data().size() != 1) {


this use of the raw data map scares me.. all the key validation that's required

jjoyce0510 · 2021-12-10T23:21:16Z

...ce/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestRetentionPoliciesStep.java

+  @Nonnull
+  @Override
+  public ExecutionMode getExecutionMode() {
+    return ExecutionMode.ASYNC;


is there any concern with allowing datahub to finish booting up before running retention? like any race conditions with ingestion or other bootstrap steps we need to be concerned about?

I don't expect any since version 0 is never deleted

jjoyce0510 · 2021-12-10T23:59:45Z

metadata-io/src/main/java/com/linkedin/metadata/entity/ebean/EbeanRetentionService.java

+    log.info("Finished applying retention to all records");
+  }
+
+  private Map<String, DataHubRetentionInfo> getRetentionPolicies() {


let's call this getAllRetentionPolicies

…ention

jjoyce0510 · 2021-12-13T18:55:08Z

...s/src/main/java/com/linkedin/metadata/boot/factories/IngestRetentionPoliciesStepFactory.java

+  @Qualifier("retentionService")
+  private RetentionService _retentionService;
+
+  @Value("${entityService.retention.enable}")


Sorry to nit on this again - I think the others (analytics, auth) use "enabled"

jjoyce0510 · 2021-12-13T18:56:28Z

metadata-service/factories/src/main/resources/application.yml

+
+entityService:
+  retention:
+    enable: ${ENTITY_SERVICE_ENABLE_RETENTION:false}


enabled to be consistent

jjoyce0510 · 2021-12-13T18:56:39Z

metadata-service/war/src/main/resources/boot/retention.yaml

@@ -0,0 +1,14 @@
+- entity: "*"


looks great!

jjoyce0510

Overall looks great - just one final nit on the enabled config.

shirshanka

LGTM

Dexter Lee added 21 commits December 4, 2021 15:57

In progress

1cbc500

In progress

686c623

Merge remote-tracking branch 'origin/master' into dl--add-retention

ad7276a

In progress

3e3d00c

Small fix

f41824f

Add datahub upgrade job for applying retention to all

905275d

Done

fa5791a

Revert datahub-upgrade

94454eb

Add delete

4ec1f38

Add delete retention

3a78294

Improve batch apply retention

e3d2f7b

Merge remote-tracking branch 'origin/master' into dl--add-retention

367b4da

In progress

9a5c86b

In progress

8986da0

Finished

1a42256

Add logs

f4572d6

Finished

c3a13a7

Add docs

c1a1006

checkstyle :(

a22b3b0

Fix checkstyle

bb171e8

Add retention for entity service

6ca8463

jjoyce0510 reviewed Dec 10, 2021

View reviewed changes

docs/advanced/db-retention.md

@@ -0,0 +1,79 @@

# Configuring Database Retention

## Goal

Copy link

Collaborator

jjoyce0510 Dec 10, 2021

Choose a reason for hiding this comment

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

Great doc!

jjoyce0510 reviewed Dec 10, 2021

View reviewed changes

dexter-mh-lee commented Dec 10, 2021

View reviewed changes

jjoyce0510 reviewed Dec 10, 2021

View reviewed changes

Dexter Lee added 9 commits December 11, 2021 10:40

Fix to John's comments

5d1d306

Comment it out

77fa7cb

Checkstyle

612d9b9

Merge remote-tracking branch 'private/dl--add-retention' into dl--ret…

7da74b4

…ention

Merge remote-tracking branch 'origin/master' into dl--add-retention

86a834b

Merge remote-tracking branch 'origin/master' into dl--retention

2152e5b

Fix

6ef3a5f

Fix test

9d69fae

Merge remote-tracking branch 'private/dl--add-retention' into dl--ret…

c7cdf99

…ention

jjoyce0510 reviewed Dec 13, 2021

View reviewed changes

metadata-service/war/src/main/resources/boot/retention.yaml

@@ -0,0 +1,14 @@

- entity: "*"

Copy link

Collaborator

jjoyce0510 Dec 13, 2021

Choose a reason for hiding this comment

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

looks great!

jjoyce0510 approved these changes Dec 13, 2021

View reviewed changes

Fix to John's comments

fb83735

shirshanka approved these changes Dec 14, 2021

View reviewed changes

shirshanka merged commit 8e4769f into datahub-project:master Dec 14, 2021

feat(retention): Add retention to Local DB #3715

feat(retention): Add retention to Local DB #3715

Uh oh!

Conversation

dexter-mh-lee commented Dec 10, 2021

Checklist

Uh oh!

github-actions bot commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Dec 10, 2021 •

edited

Loading