-
Notifications
You must be signed in to change notification settings - Fork 134
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
Cancel or delete stale pending and failed exports #1086
Conversation
Fixes: #1068 Create periodic tasks which: - Find old pending exports and mark them as failed. - Find old failed exports and delete them The 'oldness' is determined by a configurable setting.
onadata/apps/viewer/tasks.py
Outdated
Delete old failed exports | ||
""" | ||
h = settings.EXPORT_TASK_LIFESPAN | ||
time_threshold = datetime.now() - timedelta(hours=h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably use the time zone aware django.utils.timezone.now()
.
The h
could probably be more descriptive like task_lifespan
.
onadata/apps/viewer/tasks.py
Outdated
exports = Export.objects.filter(internal_status=Export.FAILED, | ||
created_on__lt=time_threshold) | ||
for export in exports: | ||
delete_export.delay(export.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you chose to delete the export in a different task considering this is already task? Possible use queryset
deletion, exports.delet()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had seen that there was a task created to specifically delete exports that was used elsewhere, and so I chose to use it - to keep things consistent.
|
||
def test_check_pending_exports(self): | ||
result = check_pending_exports.delay() | ||
self.assertTrue(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably check that the function deletes exports and not that a task is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. And implemented.
Improve tests to not only test that the celery tasks below are working: - onadata.apps.viewer.tasks.check_pending_exports - onadata.apps.viewer.tasks.delete_old_failed_exports But also test that they have the desired effect on pending and failed exports, respectively.
Fixes: #1068
Create periodic tasks which:
Find old pending exports and mark them as failed.
Find old failed exports and delete them
The 'oldness' is determined by a configurable setting.