1983 - Improve sync optimization to do a full sync after config file change or content removal #2637
Conversation
|
@dralley, I'm going to assign you until you are ready for review. |
| New Features | ||
| ------------ | ||
|
|
||
| * Re-sync of the repository will be a no operation by default if since the last sync, no units |
There was a problem hiding this comment.
@dralley re-sync of the repository cannot be no operational, therefore have skipped result based on your code and the feature requirements. The improvements that you did will do always an operational sync, the difference is - sometimes it will perform all sync steps without skipping any and sometimes it will take advantage of sync optimization logic and skip some of the steps. What you did is - you moved part of logic into the platform, so we first will look into platform at some conditions, if those conditions are true then we will disregard the rest of logic optimization that is done in each plugin. and do a so-called force sync.
There was a problem hiding this comment.
Would you say that since there is no change in behavior from a user perspective (logic has simply been moved rather than added, necessarily), it doesn't need a release note at all?
I definitely agree that it's incorrect as it exists currently - if it does need a note I'll rewrite it to be more accurate.
There was a problem hiding this comment.
it did not change from user perspective, but i think it still worth to mention that there is behaviour change, that if the content was removed from last sync or config was updated + other conditions that were moved to platform, then a force-sync will be triggered
There was a problem hiding this comment.
I will fix it in my next push.
|
@dralley back to you, i did not check the unittests though, i will do that once the functional code will be updated/ready. |
|
@dralley one more suggestion about the commit message length. |
|
@dralley don't forget to provide the link to your PRs in the issue https://pulp.plan.io/issues/1983 |
| the previous sync. | ||
| :rtype: boolean | ||
| """ | ||
| repo_importer = model.Importer.objects.get_or_404(repo_id=repo_id) |
There was a problem hiding this comment.
I am not too worried about repeated db calls since this occurs at the start of a sync (instead of say, for each package in a sync), but you could reduce the number of call by including the repo_importer as a param.
|
@dralley i finished the review, there are couple of minor suggestions that do not require any re-review. |
| @@ -816,6 +820,124 @@ def sync(repo_id, sync_config_override=None, scheduled_call_id=None): | |||
| return TaskResult(sync_result, spawned_tasks=spawned_tasks) | |||
|
|
|||
|
|
|||
| def check_unit_removed_since_last_sync(conduit, repo_id): | |||
| """ | |||
| Checks whether the a content unit has been removed since the last_sync timestamp. | |||
dralley
changed the title
should be forced to the repo controller. closes #1983
Add full sync checks to the platform, where they can be used by all plugins.
closes #1983
https://pulp.plan.io/issues/1983