OSIDB-3579: Implement alert versioning to reduce database locks #856
Conversation
MrMarble
force-pushed
the
OSIDB-3579
branch
4 times, most recently
from
eb3756c to
985bb5e
Compare
MrMarble
changed the title
|
|
||
| logger.info(f"Found {filtered_queryset.count()} stale alerts") | ||
|
|
||
| deleted_alerts_count = filtered_queryset.delete()[0] |
There was a problem hiding this comment.
I am thinking about atomicity, do we need to somehow enforce it here? 🤔
There was a problem hiding this comment.
Well I'm not sure about it, alerts are not modified once they go stale, so nothing should conflict
There was a problem hiding this comment.
This job should be the only piece of code performing write operations on the stale alerts so there should be no danger of any conflict. I do not see a reason for handling the atomicity. And as the delete is performed on the queryset I would expect that it is atomic anyway 😄
MrMarble
force-pushed
the
OSIDB-3579
branch
2 times, most recently
from
3d7faf0 to
25f3bf4
Compare
| class OSIDBScheduler(AppConfig): | ||
| """OSIDB scheduler service""" | ||
|
|
||
| name = "collectors.osidb_scheduler" |
There was a problem hiding this comment.
I do not think that collectors is the right place. The collectors are meant to bring some data to OSIDB from external sources which does not happen here. I assume that this module should run rather internal tasks like housekeeping here or some checks but not really a data collection.
There was a problem hiding this comment.
Sorry, that is probably my fault as I pointed @MrMarble this way. I am aware that we don't yet have anything in OSIDB being scheduled to do work periodically without downloading data from anywhere so my initial thought was to just reuse the collectors framework.
| logger = get_task_logger(__name__) | ||
|
|
||
|
|
||
| class StaleAlertCollector(Collector): |
There was a problem hiding this comment.
The collector framework is too have for this periodic job. It was designed for the data collection so the model has attributes like collected data status, up until when the data is complete, etc. while nothing like this is necessary here. You just perform full cleanup every time this job runs and that is it. No information to store needed, no status etc. Just a simple periodic Celery task should be enough or not? We do not have any framework for simple internal tasks like this (to eg. report status on API if needed etc.) but also we currently do not have a need for one 😄 so I would think that Celery should be enough.
|
|
||
| logger.info(f"Found {filtered_queryset.count()} stale alerts") | ||
|
|
||
| deleted_alerts_count = filtered_queryset.delete()[0] |
There was a problem hiding this comment.
This job should be the only piece of code performing write operations on the stale alerts so there should be no danger of any conflict. I do not see a reason for handling the atomicity. And as the delete is performed on the queryset I would expect that it is atomic anyway 😄
| @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ## Unreleased | |||
| ### Changed | |||
| - Use keywords from ps-constants in CVEorg collector (OSIDB-3694) | |||
| - Added last_validated_dt field to AlertMixin and created_dt to alerts; | |||
| optimized alert handling by matching valid alerts via timestamps | |||
| and scheduling stale alert removal to reduce database locks. (OSIDB-3579) | |||
There was a problem hiding this comment.
nitpick (feel free to ignore): The CHANGELOG should be primarily for the user (the developers have Git history) and I am not sure they could have an idea what those attribute names stand for 😄 So describing the reasons or effects of the change is more important - or sometimes the CHANGELOG record could be IMO fully omitted if we do some purely internal changes which do not affect the user or in some totally indirect way.
last_validated_dtto theAlertMixinandcreated_dtto theAlertModel.alerts.all.delete()method call to prevent database locksAlertSerializerto filter out alerts with acreated_dtolder than thelast_validated_dtof the object they are relatedStaleAlertCollectorcollector to clean up stale alerts every hourCloses OSIDB-3579