[RFC,1/4] Automatically remove old events

Message ID 20180110004024.5551-2-stephen@that.guru
State RFC
Headers show
Series
  • Add 'Event.payload' field
Related show

Commit Message

Stephen Finucane Jan. 10, 2018, 12:40 a.m.
Events are created for most state changes in Patchwork which means
they're fairly numerous. Start removing them after a (configurable)
interval.

Signed-off-by: Stephen Finucane <stephen@that.guru>
---
 docs/deployment/configuration.rst                      |  7 +++++++
 docs/usage/overview.rst                                |  2 ++
 patchwork/management/commands/cron.py                  |  2 ++
 patchwork/notifications.py                             | 18 ++++++++++++++----
 patchwork/settings/base.py                             |  3 +++
 ...tomatically-remove-old-events-4ee222aaf4a6ea6c.yaml |  8 ++++++++
 6 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml

Comments

Daniel Axtens Jan. 25, 2018, 5:21 a.m. | #1
Hi Stephen,

This is a good idea and I'm happy for it to go in as-is, before the rest
of the series.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel

> Events are created for most state changes in Patchwork which means
> they're fairly numerous. Start removing them after a (configurable)
> interval.
>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> ---
>  docs/deployment/configuration.rst                      |  7 +++++++
>  docs/usage/overview.rst                                |  2 ++
>  patchwork/management/commands/cron.py                  |  2 ++
>  patchwork/notifications.py                             | 18 ++++++++++++++----
>  patchwork/settings/base.py                             |  3 +++
>  ...tomatically-remove-old-events-4ee222aaf4a6ea6c.yaml |  8 ++++++++
>  6 files changed, 36 insertions(+), 4 deletions(-)
>  create mode 100644 releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml
>
> diff --git a/docs/deployment/configuration.rst b/docs/deployment/configuration.rst
> index 34748563..3494bd39 100644
> --- a/docs/deployment/configuration.rst
> +++ b/docs/deployment/configuration.rst
> @@ -51,6 +51,13 @@ This is customizable on a per-user basis from the user configuration page.
>      This option was previously named ``DEFAULT_PATCHES_PER_PAGE``. It was
>      renamed as cover letters are now supported also.
>  
> +``EVENT_VALIDITY_DAYS``
> +~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The number of days to retain :ref:`events` for. After this interval, the
> +:ref:`cron management command <deployment-final-steps>` will delete the
> +request.
> +
>  ``CONFIRMATION_VALIDITY_DAYS``
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
> index 802de2f1..7ac265f7 100644
> --- a/docs/usage/overview.rst
> +++ b/docs/usage/overview.rst
> @@ -207,6 +207,8 @@ To-do Lists
>  
>  Patchwork users can store a to-do list of patches.
>  
> +.. _events:
> +
>  Events
>  ------
>  
> diff --git a/patchwork/management/commands/cron.py b/patchwork/management/commands/cron.py
> index 2be234b1..653fb0b3 100644
> --- a/patchwork/management/commands/cron.py
> +++ b/patchwork/management/commands/cron.py
> @@ -19,6 +19,7 @@
>  
>  from django.core.management.base import BaseCommand
>  
> +from patchwork.notifications import expire_events
>  from patchwork.notifications import expire_notifications
>  from patchwork.notifications import send_notifications
>  
> @@ -34,3 +35,4 @@ class Command(BaseCommand):
>                                (recipient.email, error))
>  
>          expire_notifications()
> +        expire_events()
> diff --git a/patchwork/notifications.py b/patchwork/notifications.py
> index 88e96628..b604d579 100644
> --- a/patchwork/notifications.py
> +++ b/patchwork/notifications.py
> @@ -1,5 +1,6 @@
>  # Patchwork - automated patch tracking system
>  # Copyright (C) 2008 Jeremy Kerr <jk@ozlabs.org>
> +# Copyright (C) 2017 Stephen Finucane <stephen@that.guru>
>  #
>  # This file is part of the Patchwork package.
>  #
> @@ -31,6 +32,7 @@ from django.template.loader import render_to_string
>  
>  from patchwork.models import EmailConfirmation
>  from patchwork.models import EmailOptout
> +from patchwork.models import Event
>  from patchwork.models import PatchChangeNotification
>  
>  
> @@ -109,8 +111,16 @@ def expire_notifications():
>      EmailConfirmation.objects.filter(q).delete()
>  
>      # remove inactive users with no pending confirmation
> -    pending_confs = EmailConfirmation.objects.values('user')
> -    users = User.objects.filter(is_active=False).exclude(id__in=pending_confs)
> +    pending_conf = EmailConfirmation.objects.values('user')
> +    User.objects.filter(is_active=False).exclude(id__in=pending_conf).delete()
>  
> -    # delete users
> -    users.delete()
> +
> +def expire_events():
> +    """Drop excess events.
> +
> +    Events are generated for almost everything and shouldn't be allowed
> +    to grow unchecked. This task cleans them up.
> +    """
> +    event_validity = datetime.timedelta(days=settings.EVENTS_VALIDITY_DAYS)
> +    q = Q(date__lt=datetime.datetime.now() - event_validity)
> +    Event.objects.filter(q).delete()
> diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
> index 4b0d5513..30734ece 100644
> --- a/patchwork/settings/base.py
> +++ b/patchwork/settings/base.py
> @@ -204,9 +204,12 @@ LOGGING = {
>  #
>  # Patchwork settings
>  #
> +# https://patchwork.readthedocs.io/en/latest/deployment/configuration/
>  
>  DEFAULT_ITEMS_PER_PAGE = 100
>  
> +EVENTS_VALIDITY_DAYS = 14
> +
>  CONFIRMATION_VALIDITY_DAYS = 7
>  
>  NOTIFICATION_DELAY_MINUTES = 10
> diff --git a/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml b/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml
> new file mode 100644
> index 00000000..92590e49
> --- /dev/null
> +++ b/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml
> @@ -0,0 +1,8 @@
> +---
> +features:
> +  - |
> +    The ``EVENTS_VALIDITY_DAYS`` configuration option is added. This configures
> +    the number of days that an event should be retained for.
> +  - |
> +    The ``cron`` management command will now remove events older than
> +    ``EVENTS_VALIDITY_DAYS`` days.
> -- 
> 2.14.3
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Stephen Finucane Feb. 6, 2018, 8:41 p.m. | #2
On Thu, 2018-01-25 at 16:21 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> This is a good idea and I'm happy for it to go in as-is, before the
> rest
> of the series.
> 
> Reviewed-by: Daniel Axtens <dja@axtens.net>

I've been thinking a little more on this and I don't know if I actually
want to do this. If we manage to solve the performance impact, we can
use events as an auditor and display things like state changes on the
patch detail page. I might leave this one for another while. Would
still appreciate reviews on the rest of this series though.

Stephen

> 
> Regards,
> Daniel
Daniel Axtens March 12, 2018, 3:48 a.m. | #3
Stephen Finucane <stephen@that.guru> writes:

> On Thu, 2018-01-25 at 16:21 +1100, Daniel Axtens wrote:
>> Hi Stephen,
>> 
>> This is a good idea and I'm happy for it to go in as-is, before the
>> rest
>> of the series.
>> 
>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> I've been thinking a little more on this and I don't know if I actually
> want to do this. If we manage to solve the performance impact, we can
> use events as an auditor and display things like state changes on the
> patch detail page. I might leave this one for another while. Would
> still appreciate reviews on the rest of this series though.

I think having this configurable would be the best. Perhaps have
0 = disable, 1+ = delete after N days.

(Maybe we need to expose a unique ID for patches to help people who use
them.)

Regards,
Daniel

>
> Stephen
>
>> 
>> Regards,
>> Daniel
Daniel Axtens March 12, 2018, 8:49 a.m. | #4
Daniel Axtens <dja@axtens.net> writes:

> Stephen Finucane <stephen@that.guru> writes:
>
>> On Thu, 2018-01-25 at 16:21 +1100, Daniel Axtens wrote:
>>> Hi Stephen,
>>> 
>>> This is a good idea and I'm happy for it to go in as-is, before the
>>> rest
>>> of the series.
>>> 
>>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>>
>> I've been thinking a little more on this and I don't know if I actually
>> want to do this. If we manage to solve the performance impact, we can
>> use events as an auditor and display things like state changes on the
>> patch detail page. I might leave this one for another while. Would
>> still appreciate reviews on the rest of this series though.
>
> I think having this configurable would be the best. Perhaps have
> 0 = disable, 1+ = delete after N days.
>
> (Maybe we need to expose a unique ID for patches to help people who use
> them.)
As it turns out, we already do this.

>
> Regards,
> Daniel
>
>>
>> Stephen
>>
>>> 
>>> Regards,
>>> Daniel

Patch

diff --git a/docs/deployment/configuration.rst b/docs/deployment/configuration.rst
index 34748563..3494bd39 100644
--- a/docs/deployment/configuration.rst
+++ b/docs/deployment/configuration.rst
@@ -51,6 +51,13 @@  This is customizable on a per-user basis from the user configuration page.
     This option was previously named ``DEFAULT_PATCHES_PER_PAGE``. It was
     renamed as cover letters are now supported also.
 
+``EVENT_VALIDITY_DAYS``
+~~~~~~~~~~~~~~~~~~~~~~~
+
+The number of days to retain :ref:`events` for. After this interval, the
+:ref:`cron management command <deployment-final-steps>` will delete the
+request.
+
 ``CONFIRMATION_VALIDITY_DAYS``
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/docs/usage/overview.rst b/docs/usage/overview.rst
index 802de2f1..7ac265f7 100644
--- a/docs/usage/overview.rst
+++ b/docs/usage/overview.rst
@@ -207,6 +207,8 @@  To-do Lists
 
 Patchwork users can store a to-do list of patches.
 
+.. _events:
+
 Events
 ------
 
diff --git a/patchwork/management/commands/cron.py b/patchwork/management/commands/cron.py
index 2be234b1..653fb0b3 100644
--- a/patchwork/management/commands/cron.py
+++ b/patchwork/management/commands/cron.py
@@ -19,6 +19,7 @@ 
 
 from django.core.management.base import BaseCommand
 
+from patchwork.notifications import expire_events
 from patchwork.notifications import expire_notifications
 from patchwork.notifications import send_notifications
 
@@ -34,3 +35,4 @@  class Command(BaseCommand):
                               (recipient.email, error))
 
         expire_notifications()
+        expire_events()
diff --git a/patchwork/notifications.py b/patchwork/notifications.py
index 88e96628..b604d579 100644
--- a/patchwork/notifications.py
+++ b/patchwork/notifications.py
@@ -1,5 +1,6 @@ 
 # Patchwork - automated patch tracking system
 # Copyright (C) 2008 Jeremy Kerr <jk@ozlabs.org>
+# Copyright (C) 2017 Stephen Finucane <stephen@that.guru>
 #
 # This file is part of the Patchwork package.
 #
@@ -31,6 +32,7 @@  from django.template.loader import render_to_string
 
 from patchwork.models import EmailConfirmation
 from patchwork.models import EmailOptout
+from patchwork.models import Event
 from patchwork.models import PatchChangeNotification
 
 
@@ -109,8 +111,16 @@  def expire_notifications():
     EmailConfirmation.objects.filter(q).delete()
 
     # remove inactive users with no pending confirmation
-    pending_confs = EmailConfirmation.objects.values('user')
-    users = User.objects.filter(is_active=False).exclude(id__in=pending_confs)
+    pending_conf = EmailConfirmation.objects.values('user')
+    User.objects.filter(is_active=False).exclude(id__in=pending_conf).delete()
 
-    # delete users
-    users.delete()
+
+def expire_events():
+    """Drop excess events.
+
+    Events are generated for almost everything and shouldn't be allowed
+    to grow unchecked. This task cleans them up.
+    """
+    event_validity = datetime.timedelta(days=settings.EVENTS_VALIDITY_DAYS)
+    q = Q(date__lt=datetime.datetime.now() - event_validity)
+    Event.objects.filter(q).delete()
diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
index 4b0d5513..30734ece 100644
--- a/patchwork/settings/base.py
+++ b/patchwork/settings/base.py
@@ -204,9 +204,12 @@  LOGGING = {
 #
 # Patchwork settings
 #
+# https://patchwork.readthedocs.io/en/latest/deployment/configuration/
 
 DEFAULT_ITEMS_PER_PAGE = 100
 
+EVENTS_VALIDITY_DAYS = 14
+
 CONFIRMATION_VALIDITY_DAYS = 7
 
 NOTIFICATION_DELAY_MINUTES = 10
diff --git a/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml b/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml
new file mode 100644
index 00000000..92590e49
--- /dev/null
+++ b/releasenotes/notes/automatically-remove-old-events-4ee222aaf4a6ea6c.yaml
@@ -0,0 +1,8 @@ 
+---
+features:
+  - |
+    The ``EVENTS_VALIDITY_DAYS`` configuration option is added. This configures
+    the number of days that an event should be retained for.
+  - |
+    The ``cron`` management command will now remove events older than
+    ``EVENTS_VALIDITY_DAYS`` days.