Skip to content
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.

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

update respective index with the assets #13843

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

07Himank
Copy link
Contributor

@07Himank 07Himank commented Nov 3, 2023

I worked on ... because ... update respective index with the assets

Type of change:

  • [ x] Bug fix
  • [ x] Improvement

@07Himank 07Himank added the safe to test Add this label to run secure Github workflows on PRs label Nov 3, 2023
@07Himank 07Himank requested a review from a team as a code owner November 3, 2023 12:21
Copy link
Contributor

github-actions bot commented Nov 3, 2023

The Java checkstyle failed.

Please run mvn googleformatter:format@reformat-sources in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Java code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

@@ -114,10 +116,14 @@ private void updateAssets() {
// Remove assets that were deleted
for (EntityReference asset : deleted) {
deleteRelationship(original.getId(), DATA_PRODUCT, asset.getId(), asset.getType(), Relationship.HAS);
EntityInterface entityInterface = Entity.getEntity(asset, "*", Include.ALL);
searchRepository.updateEntity(entityInterface);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you calling this inline, why can't we do this in post updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the change, @karanh37 can you please verify from you end is it working as expected ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Verified on UI, changes are working fine for Data Product.

Copy link

sonarqubecloud bot commented Nov 6, 2023

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

listOrEmpty(original.getAssets()).forEach(asset -> assetsMap.put(asset.getId().toString(), asset));
listOrEmpty(updated.getAssets()).forEach(asset -> assetsMap.put(asset.getId().toString(), asset));
for (EntityReference assetRef : assetsMap.values()) {
EntityInterface asset = Entity.getEntity(assetRef, "*", Include.ALL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we making this call?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to penalize the post/put operations on data product as we are making additional calls. Also it looks like this will be triggered on any update to the data product such as description updates , ownership cc @mohityadav766

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harshach , beacuse of this i was thinking and added it to the only in entitySpicificUpdates while doing updateAssets to avoid unnecessary calls. in this as we need to update the indexes of assets which are added, removed or update.

@mohityadav766 mohityadav766 merged commit 1a14ac6 into main Nov 8, 2023
@mohityadav766 mohityadav766 deleted the updateAssetsRelationshipDataProduct branch November 8, 2023 05:06
07Himank added a commit that referenced this pull request Nov 8, 2023
* update respective index with the assets

* checkstyle

* addressing comments

* add listorempty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend safe to test Add this label to run secure Github workflows on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants