fix(ingest/gc): add limit, add actual loop for iterating over batches #11809
Conversation
github-actions
bot
added
the
ingestion
|
Hello @anshbansal 😄 Thank you so much for opening a pull request!
|
| @@ -59,6 +59,9 @@ class SoftDeletedEntitiesCleanupConfig(ConfigModel): | |||
| default=None, | |||
| description="Query to filter entities", | |||
| ) | |||
| limit_entities_delete: int = Field( | |||
There was a problem hiding this comment.
Can we make this optional?
| self.report.num_soft_deleted_entity_removed | ||
| <= self.config.limit_entities_delete | ||
| ): | ||
| urns = list( |
There was a problem hiding this comment.
You get here more items than the limit.
I think it would be better to only iterate over the get_urns_by_filter and stop when you hit the limit and only keep limit number of item in memory and in the list
There was a problem hiding this comment.
Do you mean in the case where batch_size > limit?
There was a problem hiding this comment.
What if I do this
while (
- self.report.num_soft_deleted_entity_removed
- <= self.config.limit_entities_delete
+ self.config.limit_entities_delete <= 0
+ or self.report.num_soft_deleted_entity_removed
+ <= max(self.config.limit_entities_delete, self.config.batch_size)
):
| 10000, description="Max number of entities to delete." | ||
| ) | ||
|
|
||
| runtime_limit_seconds: Optional[int] = Field( |
There was a problem hiding this comment.
Think this makes more sense as minutes
There was a problem hiding this comment.
probably true in reality but dev-ops is pretty used to dealing with seconds for lots of things, feel free to change it later but not a blocker imho
Checklist