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

[Framework] Improvements to delete netities on real time #571

Merged
merged 14 commits into from
Apr 24, 2024

Conversation

omby8888
Copy link
Contributor

@omby8888 omby8888 commented Apr 18, 2024

Description

What - Improvements to delete entities on real time, denying inefficient cases
Why - previous changes might resulted a case of trying to calculate full jq mapping of entities that wouldn't being registered eventually, and also performing lots of api requests to port api.
How - using multiple rules search api when checking entities to delete and calculating only relevant for deletion jq fields

Type of change

Please leave one option from the following and delete the rest:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New Integration (non-breaking change which adds a new integration)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Non-breaking change (fix of existing functionality that will not change current behavior)
  • Documentation (added/updated documentation)

Screenshots

Include screenshots from your environment showing how the resources of the integration will look.

API Documentation

Provide links to the API documentation used for this integration.

@omby8888 omby8888 changed the title Improvements to delete netities on real time [Framework] Improvements to delete netities on real time Apr 18, 2024
Comment on lines 207 to 216
{
"property": "$identifier",
"operator": "contains",
"value": entity.identifier,
},
{
"property": "$blueprint",
"operator": "contains",
"value": entity.blueprint,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{
"property": "$identifier",
"operator": "contains",
"value": entity.identifier,
},
{
"property": "$blueprint",
"operator": "contains",
"value": entity.blueprint,
},
{
"property": "$identifier",
"operator": "=",
"value": entity.identifier,
},
{
"property": "$blueprint",
"operator": "=",
"value": entity.blueprint,
},

@@ -209,6 +195,37 @@ async def _register_in_batches(
)
return entities, errors

async def _search_deleted_entities(
Copy link
Contributor

Choose a reason for hiding this comment

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

you are not searching deleted entities, you are searching entities to delete

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont agree with that either
Nothing in this function says delete

Comment on lines +253 to 259
diffs: list[EntitySelectorDiff] = await asyncio.gather(
*(
self._register_resource_raw(resource, results, user_agent_type, True)
for resource in resource_mappings
)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

will this be optimized in your other PR? as there could be thousand of entities at once and spawning all can have a huge decrease in performance and take a lot of memory

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did he change anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i didn't changed anything there, if it causes weak performance i'll handle it in the other PR but there isn't suppose to be any new performance issue from the current PR

Comment on lines 279 to 282
logger.info(
f"Skipping deletion for {len(entities_to_delete) - len(filtered_entities_to_delete)} entities "
f"as we couldn't find matching entities that's related to the current integration."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

could be very spamming message, as it will happen most of the time probably. maybe just add a log before the deletion, that you are going to perform a delete

CHANGELOG.md Outdated
Comment on lines 13 to 14

- Delete entities on real time events improvements
Copy link
Contributor

Choose a reason for hiding this comment

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

explain the improvements

*(
self._register_resource_raw(resource, results, user_agent_type, True)
for resource in resource_mappings
)
)

registered_entities: list[Entity] = [
item for d in diffs for item in d["passed"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

D?

*(
self._register_resource_raw(resource, results, user_agent_type, True)
for resource in resource_mappings
)
)

registered_entities: list[Entity] = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use zip to create 2 lists instead of 3 double iteration

Copy link
Collaborator

@yairsimantov20 yairsimantov20 left a comment

Choose a reason for hiding this comment

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

more comments

CHANGELOG.md Outdated Show resolved Hide resolved
port_ocean/core/integrations/mixins/sync_raw.py Outdated Show resolved Hide resolved
port_ocean/core/integrations/mixins/sync_raw.py Outdated Show resolved Hide resolved
port_ocean/core/integrations/mixins/sync_raw.py Outdated Show resolved Hide resolved
port_ocean/core/integrations/mixins/sync_raw.py Outdated Show resolved Hide resolved
port_ocean/core/ocean_types.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yairsimantov20 yairsimantov20 left a comment

Choose a reason for hiding this comment

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

🌊

port_ocean/core/integrations/mixins/sync_raw.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 14 to 15
- Delete entities on real time events only if they didn't pass any of the selectors
- Calculated JQ of only identifier and blueprint to raw entities that didn't pass any selector on real time events
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Delete entities on real time events only if they didn't pass any of the selectors
- Calculated JQ of only identifier and blueprint to raw entities that didn't pass any selector on real time events
- Implemented real-time entity deletion exclusively for instances that haven't matched any selectors.
- Change the JQ calculation to process only identifier and blueprint for raw entities not selected during real-time events to only get the required data for the delete.

@omby8888 omby8888 merged commit 5e19fd0 into main Apr 24, 2024
4 checks passed
@omby8888 omby8888 deleted the delete-entity-on-real-time-events-improvements branch April 24, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants