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.

Sign up for GitHub

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

Jump to bottom

implement dual write in BaseAspectRoutingResource #420

Merged

jsdonn merged 5 commits into linkedin:master from jsdonn:dual-write

Sep 16, 2024

Contributor

jsdonn commented Sep 11, 2024 •

edited

Loading

Summary

Change the behavior of BaseAspectRoutingResource's ingestInternal method to write any routed aspects locally in addition to routing the request to another GMS.

Details

Update ingestInternal and ingestAssetInternal with the same logic
Move resource-level pre-ingestion lambdas into the if statement which checks if an aspect has a registered routing client.
In the aspect routing branch of the if statement, also call a new local DAO API addSimple which will store the aspect locally without executing any pre-ingestion lambdas within BaseLocalDAO. This is to prevent the case where a pre-ingestion lambda is executed twice in one ingestion (one before the routing, one within the local DAO).

Testing Done

Update existing unit tests to expect dual-write behavior. Added some comments for context.

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)

ybz1013 reviewed

View reviewed changes

Contributor

ybz1013 left a comment

I dont really have the context from #410. Just left some comments.

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

+                 * Please use the regular add method linked above.
+                 */
+                @Nonnull
+                public <ASPECT extends RecordTemplate> ASPECT addSimple(@Nonnull URN urn, @Nonnull ASPECT newValue,

Contributor

ybz1013 Sep 11, 2024

can we further reduce duplicate code (864-866) by adding a param like isPreupdateLambasEnabled in a new add function? Then the old add and this addSimple can both call that.

Contributor

ybz1013 Sep 11, 2024

after looking at #410, this addSimple looks the same as the old add function.

Contributor

hongliu2024 Sep 11, 2024

+1 to the suggestion.
Also suggest to rename addSimple to something for descriptive maybe addWithoutPreIngestionLamdba?

Contributor Author

jsdonn Sep 16, 2024

Changed to addSkipPreIngestionUpdates

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated

                     final AuditStamp auditStamp = getAuditor().requestAuditStamp(getContext().getRawRequestContext());
                     ModelUtils.getAspectsFromSnapshot(snapshot).forEach(aspect -> {
                       if (!aspectsToIgnore.contains(aspect.getClass())) {
-                        if (_restliPreUpdateAspectRegistry != null && _restliPreUpdateAspectRegistry.isRegistered(
-                            aspect.getClass())) {
-                          aspect = preUpdateRouting(urn, aspect);

Contributor

ybz1013 Sep 11, 2024

question: does this mean the old code will execute preUpdateRouting twice if the aspect is registered in the _restliPreUpdateAspectRegistry AND not a routed aspect? Once here, and once in the add function.

Contributor Author

jsdonn Sep 11, 2024

Yes exactly, old code would have executed preUpdateRouting twice in the case you mentioned.

rakhiagr requested changes

View reviewed changes

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated

-                        if (_restliPreUpdateAspectRegistry != null && _restliPreUpdateAspectRegistry.isRegistered(
-                            aspect.getClass())) {
-                          aspect = preUpdateRouting(urn, aspect);
-                        }
                         // Get the fqcn of the aspect class
                         String aspectFQCN = aspect.getClass().getCanonicalName();

Contributor

rakhiagr Sep 11, 2024

This was added specifically for DatasetAccountableOwnership aspect to skip the normal ingestion once the preUpdateRouting is done and not go to OwnershipGMS. Can you move this lines after we call preUpdateRouting (Line 330).

// Get the fqcn of the aspect class
          String aspectFQCN = aspect.getClass().getCanonicalName();
          //TODO: META-21112: Remove this check after adding annotations at model level; to handle SKIP/PROCEED for preUpdateRouting
          if (SKIP_INGESTION_FOR_ASPECTS.contains(aspectFQCN)) {
            return;
          }

Contributor

hongliu2024 Sep 11, 2024

DatasetAccountableOwnership is specially handled? It needs to call urn converter and then write to TMS?

Contributor

hongliu2024 Sep 11, 2024

Somehow I remember DatasetAccountableOwnership is deprecated and got replaced by AccountableOwnership?

Contributor Author

jsdonn Sep 11, 2024

@rakhiagr will do thanks for the pointer.

@hongliu2024 afaik DatasetAccountableOwnership is not deprecated, it's only used by Dataset entity type. But I don't have any doc on that, that's just what I know from before. Is the deprecation a recent thing? I checked dataset entity table schema, there's no AccountableOwnership column and also there's no MCE_Dataset_AccountableOwnership topic registered in nuage.

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated Show resolved Hide resolved

hongliu2024 reviewed

View reviewed changes

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

+                 * Please use the regular add method linked above.
+                 */
+                @Nonnull
+                public <ASPECT extends RecordTemplate> ASPECT addSimple(@Nonnull URN urn, @Nonnull ASPECT newValue,

Contributor

hongliu2024 Sep 11, 2024

+1 to the suggestion.
Also suggest to rename addSimple to something for descriptive maybe addWithoutPreIngestionLamdba?

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated Show resolved Hide resolved

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated

-                        if (_restliPreUpdateAspectRegistry != null && _restliPreUpdateAspectRegistry.isRegistered(
-                            aspect.getClass())) {
-                          aspect = preUpdateRouting(urn, aspect);
-                        }
                         // Get the fqcn of the aspect class
                         String aspectFQCN = aspect.getClass().getCanonicalName();

Contributor

hongliu2024 Sep 11, 2024

DatasetAccountableOwnership is specially handled? It needs to call urn converter and then write to TMS?

restli-resources/src/main/java/com/linkedin/metadata/restli/BaseAspectRoutingResource.java Outdated

-                        if (_restliPreUpdateAspectRegistry != null && _restliPreUpdateAspectRegistry.isRegistered(
-                            aspect.getClass())) {
-                          aspect = preUpdateRouting(urn, aspect);
-                        }
                         // Get the fqcn of the aspect class
                         String aspectFQCN = aspect.getClass().getCanonicalName();

Contributor

hongliu2024 Sep 11, 2024

Somehow I remember DatasetAccountableOwnership is deprecated and got replaced by AccountableOwnership?

jsdonn force-pushed the dual-write branch 2 times, most recently from 5fa6df4 to 298e5cc Compare

September 11, 2024 18:50

jsdonn added 4 commits

September 12, 2024 17:15


          implement dual write in BaseAspectRoutingResource

d2813ab


          fix docs for a unit test

a0f066f


          address comments: refactor addSimple -> addSkipPreIngestionUpdates

6a3164e


          rebase and add info logs for pre update lambda

d7577ff

jsdonn force-pushed the dual-write branch from 298e5cc to d7577ff Compare

September 13, 2024 01:02

hongliu2024 approved these changes

View reviewed changes

Contributor

hongliu2024 left a comment

Should we conduct some testing in EI before promoting to corp?

rakhiagr reviewed

View reviewed changes

restli-resources/src/test/java/com/linkedin/metadata/restli/BaseAspectRoutingResourceTest.java Outdated Show resolved Hide resolved


          update unit test

5ce74ea

rakhiagr approved these changes

View reviewed changes

Contributor

rakhiagr left a comment

Looks good, thank you for addressing all the comments

jsdonn merged commit af0995a into linkedin:master

2 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet