fix(policy): Use search to fetch all policies #4713

dexter-mh-lee · 2022-04-21T08:17:15Z

Currently, we use listUrns function to fetch all policies. We realized this function does not scale well when a lot of entities have been ingested.

Instead, we will start using search to fetch all policies. Caveat, we have no Searchable annotations on any fields for policies, which means that the search index is currently empty. Modified the ingestPoliciesStep to send MCLs for the existing policies if the search index is empty, so that we fill up the search index before doing any fetching.

As a side, add a new searchable field called lastUpdatedTimestamp, and set it on any updates. List policies sorts based on this field so newly upserted policies get ranked above.

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). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

github-actions · 2022-04-21T09:22:27Z

Unit Test Results (build & test)

  97 files ±0   97 suites ±0 17m 56s ⏱️ - 7m 3s
701 tests ±0 642 ✔️ ±0 59 💤 ±0 0 ❌ ±0

Results for commit 98f17c0. ± Comparison against base commit bbfc902.

♻️ This comment has been updated with latest results.

github-actions · 2022-04-21T09:27:34Z

Unit Test Results (metadata ingestion)

      5 files ±0       5 suites ±0 59m 57s ⏱️ -17s
  433 tests ±0   433 ✔️ ±  0   0 💤 ±  0 0 ❌ ±0
2 090 runs ±0 2 024 ✔️ - 20 66 💤 +20 0 ❌ ±0

Results for commit 98f17c0. ± Comparison against base commit bbfc902.

♻️ This comment has been updated with latest results.

jjoyce0510 · 2022-04-22T04:06:22Z

datahub-web-react/src/app/policy/PoliciesPage.tsx

@@ -244,6 +237,9 @@ export const PoliciesPage = () => {
            content: `Are you sure you want to remove policy?`,
            onOk() {
                deletePolicy({ variables: { urn: policy?.urn as string } }); // There must be a focus policy urn.
+                setTimeout(function () {
+                    policiesRefetch();


jjoyce0510 · 2022-04-22T04:06:31Z

datahub-web-react/src/app/policy/PoliciesPage.tsx

@@ -282,6 +278,9 @@ export const PoliciesPage = () => {
            createPolicy({ variables: { input: toPolicyInput(savePolicy) } });
        }
        message.success('Successfully saved policy.');
+        setTimeout(function () {


jjoyce0510 · 2022-04-22T04:09:53Z

metadata-models/src/main/pegasus/com/linkedin/policy/DataHubPolicyInfo.pdl

+  @Searchable = {
+    "fieldType": "DATETIME"
+  }
+  lastUpdatedTimestamp: optional long


Thoughts on lastModifiedMs -- only cause timestamp can mean a few formats, also to align with startTimeMs in the ExecutionRequestResult model that we use for sorting ingestion runs. Not a huge deal but let me know what you think

Oh I just followed the one added to Operation

Found it https://github.com/datahub-project/datahub/blob/master/metadata-models/src/main/pegasus/com/linkedin/common/Operation.pdl#L18 This is where I got it from. I think we've been using "last updated" in the UI so going to keep it like this

jjoyce0510 · 2022-04-22T04:10:45Z

metadata-service/auth-impl/src/main/java/com/datahub/authorization/DataHubAuthorizer.java

-      }
-    }
-
-    private void addPolicyToCache(final Map<String, List<DataHubPolicyInfo>> cache, final EntityResponse entityResponse) {


Nice! I love the idea of simplifying this.

jjoyce0510 · 2022-04-22T04:12:22Z

metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyFetcher.java

+        _entityClient.batchGetV2(POLICY_ENTITY_NAME, new HashSet<>(policyUrns), null, authentication);
+    return new PolicyFetchResult(policyUrns.stream()
+        .map(policyEntities::get)
+        .filter(Objects::nonNull)


qq - do we need double non null filter?

what if we just filtered for wheree apect map contains key

so just one filter that checks whether urns exist in the map and then entity response has the aspect? feel like efficiency wise this should be exactly the same

jjoyce0510 · 2022-04-22T04:16:07Z

...ata-service/factories/src/main/java/com/linkedin/metadata/boot/steps/IngestPoliciesStep.java

+   * Send MCLs for each policy to refill the policy search index
+   */
+  private void sendMCL() throws URISyntaxException {
+    log.info("Pushing MCLs for all policies");


what policies? bootstrap policies? do they already exist inside the document store?

Leaving for record. The current policy index is empty bc there are no searchable fields. So this step makes sure we reingest the existing policies so they get in the index one time

jjoyce0510 · 2022-04-22T04:18:15Z

metadata-service/restli-client/src/main/java/com/linkedin/entity/client/EntityClient.java

   * @param start start offset for search results
   * @param count max number of search results requested
   * @return Snapshot key
   * @throws RemoteInvocationException
   */
  @Nonnull
-  public SearchResult search(@Nonnull String entity, @Nonnull String input, @Nullable Filter filter, int start,
-      int count, @Nonnull Authentication authentication) throws RemoteInvocationException;
+  public SearchResult search(@Nonnull String entity, @Nonnull String input, @Nullable Filter filter,


nice! We also have a filter method which i've used for the same purpose.

jjoyce0510

overall LGTM - let's test on demo extensively before we cut a release :p

This reverts commit 8185ba4.

…b-project#4713)" (datahub-project#4725)" This reverts commit 3584d64.

* fix(policy): Use search to fetch all policies * Add updated timestamp * Change refetching logic and add timeout * Increase wait on smoke test

…t#4713)" (datahub-project#4725) This reverts commit 8185ba4.

Dexter Lee added 2 commits April 21, 2022 01:11

fix(policy): Use search to fetch all policies

ae96bea

Add updated timestamp

aefb741

maggiehays assigned jjoyce0510 Apr 21, 2022

Change refetching logic and add timeout

3aed252

jjoyce0510 reviewed Apr 22, 2022

View reviewed changes

Increase wait on smoke test

98f17c0

jjoyce0510 reviewed Apr 22, 2022

View reviewed changes

jjoyce0510 approved these changes Apr 22, 2022

View reviewed changes

dexter-mh-lee merged commit 8185ba4 into datahub-project:master Apr 22, 2022

dexter-mh-lee deleted the dl--fix-list-policies branch April 22, 2022 05:15

dexter-mh-lee pushed a commit that referenced this pull request Apr 22, 2022

Revert "fix(policy): Use search to fetch all policies (#4713)"

d4750f1

This reverts commit 8185ba4.

dexter-mh-lee mentioned this pull request Apr 22, 2022

Revert "fix(policy): Use search to fetch all policies" #4725

Merged

dexter-mh-lee pushed a commit that referenced this pull request Apr 22, 2022

Revert "fix(policy): Use search to fetch all policies (#4713)" (#4725)

3584d64

This reverts commit 8185ba4.

dexter-mh-lee pushed a commit to dexter-mh-lee/datahub that referenced this pull request Apr 22, 2022

Revert "Revert "fix(policy): Use search to fetch all policies (datahu…

328674d

…b-project#4713)" (datahub-project#4725)" This reverts commit 3584d64.

dexter-mh-lee mentioned this pull request Apr 22, 2022

fix(policies): Re-revert the policies fix + ingest documents directly to search #4733

Merged

5 tasks

maggiehays pushed a commit to maggiehays/datahub that referenced this pull request Aug 1, 2022

Revert "fix(policy): Use search to fetch all policies (datahub-projec…

3f14d06

…t#4713)" (datahub-project#4725) This reverts commit 8185ba4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(policy): Use search to fetch all policies #4713

fix(policy): Use search to fetch all policies #4713

dexter-mh-lee commented Apr 21, 2022 •

edited

Loading

github-actions bot commented Apr 21, 2022 •

edited

Loading

github-actions bot commented Apr 21, 2022 •

edited

Loading

jjoyce0510 Apr 22, 2022

jjoyce0510 Apr 22, 2022

jjoyce0510 Apr 22, 2022

dexter-mh-lee Apr 22, 2022

dexter-mh-lee Apr 22, 2022

jjoyce0510 Apr 22, 2022

jjoyce0510 Apr 22, 2022

jjoyce0510 Apr 22, 2022

jjoyce0510 Apr 22, 2022

dexter-mh-lee Apr 22, 2022

jjoyce0510 Apr 22, 2022

dexter-mh-lee Apr 22, 2022

jjoyce0510 Apr 22, 2022

jjoyce0510 left a comment

fix(policy): Use search to fetch all policies #4713

fix(policy): Use search to fetch all policies #4713

Conversation

dexter-mh-lee commented Apr 21, 2022 • edited Loading

Checklist

github-actions bot commented Apr 21, 2022 • edited Loading

Unit Test Results (build & test)

github-actions bot commented Apr 21, 2022 • edited Loading

Unit Test Results (metadata ingestion)

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

jjoyce0510 left a comment

Choose a reason for hiding this comment

dexter-mh-lee commented Apr 21, 2022 •

edited

Loading

github-actions bot commented Apr 21, 2022 •

edited

Loading

github-actions bot commented Apr 21, 2022 •

edited

Loading