retention-period: Add archiving expired messages tool by retention pe… #2172

kkanahin · 2016-11-01T17:26:44Z

…riod

Add models to store expired messages and expired user messages data after
removing from original Message and UserMessage db tables.
Add django migrations to create tables for archive models.
Add tools for moving expired messages to archive tables.
Add tools for deleting expired messages from Message and UserMessage tables.
Add tests to check archiving and deliting tools.
Add manage command to archive expired messages.

smarx · 2016-11-01T17:26:46Z

Automated message from Dropbox CLA bot

@kkanahin, it looks like you've already signed the Dropbox CLA. Thanks!

timabbott · 2016-11-02T07:08:56Z

zerver/tests/test_retention.py

+                ArchiveUserMessage.objects
+                    .filter(user_profile__realm=self.mit_realm)
+                    .order_by('id')
+                    .values_list('id', flat=True)


These tests are kinda long; might be worth writing some more test helper functions to simplify repetitive stuff.

I feel like these tests use the pattern list(foo.order_by('id').values_list('id', flat=True)) where foo is a query that maybe we should make the API here be e.g. sorted_ids(ArchiveUserMessage.objects .filter(user_profile__realm=self.mit_realm))

sure will split this tests

timabbott · 2016-11-02T07:13:10Z

Thanks for working on this @kkanahin! I did a read through this, and it's looking really good; I posted one comment above on the tests; will review more closely later this week.

To what extent do you have outstanding TODOs for this? The main things I notice are:

We still need to add the management command code for actually deleting old ArchivedFoo stuff, with support for a retention duration (would just make it a server setting in the zproject/settings.py defaults at 30 days or something; see docs/settings.md for details) of the ArchivedFoo tables.
We need to add ArchivedAttachment support (using Attachment.Messages to track whether the last message touching an attachment has been archived).

It's fine to work on those as added commits on top, though.

kkanahin · 2016-11-02T08:49:30Z

@timabbott Your TODO items are correct and will be done in next iteration.

rishig · 2016-11-08T02:53:28Z

Should we use the Django ORM for the SQL queries here?

kkanahin · 2016-11-08T03:32:32Z

@rishig Django orm doesn't support moving from one table to other in one query. It recieves all data then convert them to list (I mean bulk_create) and insert to the new table. In my methods it is done in one query withouth python processing.

timabbott · 2016-11-08T06:31:28Z

For this operation, which is probably going to be our single most expensive type of SQL operation that Zulip does, I think it totally makes sense to do it in SQL.

rishig · 2016-11-08T19:01:20Z

Makes sense. Thanks!

timabbott

This is looking pretty good @kkanahin! I posted several comments on the details; can you rebase past the recent models.py comment changes to use typing.Text instead of six.text_type?

Also, it'd be great if you added a design document in docs/retention.md, with notes on the various design considerations that we've discussed; similar in level of detail to e.g. docs/conversion.md.

timabbott · 2016-11-27T06:58:04Z

puppet/zulip/files/cron.d/remove_old_archive_data

+MAILTO=root
+
+# Remove old archive data . Time is in UTC.
+0 2 * * *   zulip cd /home/zulip/deployments/current && python manage.py remove_old_archive_data


We should put these two cron jobs in the same file; that makes it much more clear that they and related and the relative time offsets are important.

Also, can you update this to by compliant with 207cf63 (i.e. run ./manage.py)?

timabbott · 2016-11-27T06:59:25Z

zerver/lib/retention.py

+    arc_attachments.delete()
+
+
+def delete_archived_data():


I think these should have a clearer name, e.g. delete_expired_archived_data (and similarly for the attachments one)

timabbott · 2016-11-27T07:00:08Z

zerver/tests/test_retention.py

+        self.assertEqual(
+            ArchiveAttachment.objects.count(),
+            0
+        )


I don't see where this verifies that the attachments are actually deleted?

It is checked do not deleting archive attachments if exists some messages(not archived) which is linked with this archive attachments. And deleting them if all related messages are archived and can be removed.

timabbott · 2016-11-27T07:00:53Z

zproject/settings.py

@@ -1075,3 +1075,4 @@ def get_secret(key):
 PROFILE_ALL_REQUESTS = False

 CROSS_REALM_BOT_EMAILS = set(('feedback@zulip.com', 'notification-bot@zulip.com'))
+ARCHIVED_DATA_RETENTION_DAYS = 30


This needs to be user-configurable and thus should be added to DEFAULT_SETTINGS rather than set unconditionally here; see http://zulip.readthedocs.io/en/latest/settings.html for details on why.

timabbott · 2016-11-27T07:02:00Z

zerver/tests/test_retention.py

@@ -251,3 +288,39 @@ def test_archive_message_tool(self):
        self._check_archive_messages_ids_by_realm(
            exp_msgs_ids_dict['mit_msgs_ids'], self.mit_realm)

+    def test_moving_attachments_to_archive_withouth_removing(self):


this test name is mispelled

timabbott · 2016-11-27T07:03:45Z

zerver/lib/retention.py

+    # type: () -> None
+    removing_attachments = Attachment.objects.filter(
+        messages__isnull=True, id__in=ArchiveAttachment.objects.all())
+    removing_attachments.delete()


Call this attachments_to_remove.

kkanahin · 2016-12-01T15:34:58Z

@timabbott please review this pull request. I've added documentation page for retention feature and need your opinion about it. And could you review my answer for attachment test case.
Also I've added front-end part for retention period in admin settings.

timabbott · 2016-12-02T07:35:10Z

The document could use a pass for English grammar, but otherwise looks good; I can get another contributor to tweak that.

I'll review the new code in detail in the next few days, but the one feature that I think this is still missing is a plan / tools for how we would unarchive some messages in the event that we discovered there had been a bug in this logic; it seems likely we'll eventually want to do that, and I'm thinking it might be wise to write the code+tests for it now rather than when we have incident.

kkanahin · 2016-12-02T10:08:24Z

I've fixed English grammar. Regarding restoring tool I will develop it.

kkanahin · 2017-04-19T05:48:55Z

@timabbott, I've rebased this PR.

timabbott · 2017-05-30T05:50:28Z

Now that we've merged the message deletion commit, can you rebase this to integrate with those changes @kkanahin? Thanks!

kkanahin · 2017-05-30T07:11:53Z

@timabbott sure

- Add tools for moving expired messages to archive tables. - Add tools for deleting expired messages from Message and UserMessage tables. - Add tests to check archiving and deleting tools. - Add manage command to archive expired messages. Fixes zulip#106

- Add archiving attachments to archiving tool. - Add tests. Fixes zulip#106

- Add retention period for archived data to django settings. - Add method for removing archived data. - Add manage command for removing old archived data - Add test to check removing archived data. Fixes zulip#106

- Add cron job for moving expired messages with attachments to archive tables. - Add cron job for removing old archived data from db and remove attachments files. Fixes zulip#106

Fixes zulip#106

- Add tool to restore archived messages. - Add tool to restore archived user_messages. - Add tool to restore archived attachments and archived manytomany related records. - Add management console command to restore archived data by realm. - Add documentation for restoring tools - Add tests for restoring tools. Fixes zulip#106

- Add additional comments. - Rephrase comments to make them more clear. - Fix typos. Fixes zulip#106

- Rewrite retention tools with `dry-run` option. Split queries to selection part and executable part. - Add `dry-run` option to retention managment commands. - Add tests for `dry-run` option. Fixes zulip#106

kkanahin · 2017-05-31T04:19:35Z

@timabbott rebased

zulipbot · 2017-09-27T23:02:36Z

Heads up @kkanahin, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot · 2017-11-21T00:41:39Z

Heads up @kkanahin, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

DoCode · 2018-08-10T13:22:18Z

What's the current state here?

timabbott · 2018-08-10T19:19:46Z

This PR is mostly complete, but hasn't been integrated due to insufficient time for careful testing. It wouldn't be hard to rebase, but it's critical before we merge this that appropriate care is taken.

The main concrete thing I know about is that we'll need to make an archive-version of the Reaction model.

timabbott · 2019-05-20T03:14:50Z

Closing in favor of the current version #12356.

kkanahin force-pushed the retention_period branch from ba76d27 to 542bc24 Compare November 2, 2016 02:46

timabbott reviewed Nov 2, 2016

View reviewed changes

kkanahin force-pushed the retention_period branch 6 times, most recently from ade8034 to 9c944ea Compare November 7, 2016 10:56

kkanahin force-pushed the retention_period branch 2 times, most recently from 756ee71 to d441c68 Compare November 9, 2016 15:50

timabbott reviewed Nov 27, 2016

View reviewed changes

kkanahin force-pushed the retention_period branch 4 times, most recently from dad56bf to b252053 Compare December 1, 2016 15:29

kkanahin force-pushed the retention_period branch from b252053 to bfbd788 Compare December 2, 2016 04:04

kkanahin force-pushed the retention_period branch 2 times, most recently from b5d760e to 8b5f282 Compare December 2, 2016 10:05

kkanahin force-pushed the retention_period branch 2 times, most recently from 2a49c81 to 09780c0 Compare December 12, 2016 06:10

kkanahin force-pushed the retention_period branch from 2f3bb49 to 870513d Compare April 19, 2017 05:03

kkanahin force-pushed the retention_period branch 2 times, most recently from 96730af to 0159565 Compare May 4, 2017 06:55

kkanahin added 8 commits May 30, 2017 14:24

retention-period: Add archiving attachments feature.

efb57b5

- Add archiving attachments to archiving tool. - Add tests. Fixes zulip#106

retention-period: Add removing archived data feature.

8c580c7

- Add retention period for archived data to django settings. - Add method for removing archived data. - Add manage command for removing old archived data - Add test to check removing archived data. Fixes zulip#106

retention-period: Add cron jobs for archiving tools.

ea5d930

- Add cron job for moving expired messages with attachments to archive tables. - Add cron job for removing old archived data from db and remove attachments files. Fixes zulip#106

retention-period-doc: Add documentation page for retention feature

508e4dc

Fixes zulip#106

retention-period: Fix wording and typos.

eca59ba

- Add additional comments. - Rephrase comments to make them more clear. - Fix typos. Fixes zulip#106

retention-period: Add dry-run option to management commands.

ce055a4

- Rewrite retention tools with `dry-run` option. Split queries to selection part and executable part. - Add `dry-run` option to retention managment commands. - Add tests for `dry-run` option. Fixes zulip#106

kkanahin force-pushed the retention_period branch from 0159565 to ce055a4 Compare May 30, 2017 09:20

adnrs96 mentioned this pull request Jun 1, 2017

Make code base compliant with PEP-8 E-261. #5144

Closed

synicalsyntax unassigned brainwane and timabbott Jun 27, 2017

gnprice mentioned this pull request Jul 13, 2017

Message history or retention period #106

Closed

showell force-pushed the master branch from a44e7ed to a49fd44 Compare January 3, 2018 18:27

rishig assigned timabbott Jan 23, 2018

zulipbot added the has conflicts label Aug 7, 2018

timabbott mentioned this pull request May 20, 2019

Message retention policy (realm-global) #12356

Closed

7 tasks

timabbott closed this May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retention-period: Add archiving expired messages tool by retention pe… #2172

retention-period: Add archiving expired messages tool by retention pe… #2172

kkanahin commented Nov 1, 2016

smarx commented Nov 1, 2016

timabbott Nov 2, 2016

kkanahin Nov 2, 2016

timabbott commented Nov 2, 2016

kkanahin commented Nov 2, 2016

rishig commented Nov 8, 2016

kkanahin commented Nov 8, 2016 •

edited

Loading

timabbott commented Nov 8, 2016

rishig commented Nov 8, 2016 •

edited

Loading

timabbott left a comment •

edited

Loading

timabbott Nov 27, 2016

kkanahin Nov 29, 2016

timabbott Nov 27, 2016

kkanahin Nov 29, 2016

timabbott Nov 27, 2016

kkanahin Nov 28, 2016

timabbott Nov 27, 2016

kkanahin Nov 29, 2016

timabbott Nov 27, 2016

kkanahin Nov 29, 2016

timabbott Nov 27, 2016

kkanahin Nov 29, 2016

kkanahin commented Dec 1, 2016

timabbott commented Dec 2, 2016

kkanahin commented Dec 2, 2016

kkanahin commented Apr 19, 2017

timabbott commented May 30, 2017

kkanahin commented May 30, 2017

kkanahin commented May 31, 2017

zulipbot commented Sep 27, 2017

zulipbot commented Nov 21, 2017 •

edited by synicalsyntax

Loading

DoCode commented Aug 10, 2018

timabbott commented Aug 10, 2018 •

edited

Loading

timabbott commented May 20, 2019

retention-period: Add archiving expired messages tool by retention pe… #2172

retention-period: Add archiving expired messages tool by retention pe… #2172

Conversation

kkanahin commented Nov 1, 2016

smarx commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Nov 2, 2016

kkanahin commented Nov 2, 2016

rishig commented Nov 8, 2016

kkanahin commented Nov 8, 2016 • edited Loading

timabbott commented Nov 8, 2016

rishig commented Nov 8, 2016 • edited Loading

timabbott left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkanahin commented Dec 1, 2016

timabbott commented Dec 2, 2016

kkanahin commented Dec 2, 2016

kkanahin commented Apr 19, 2017

timabbott commented May 30, 2017

kkanahin commented May 30, 2017

kkanahin commented May 31, 2017

zulipbot commented Sep 27, 2017

zulipbot commented Nov 21, 2017 • edited by synicalsyntax Loading

DoCode commented Aug 10, 2018

timabbott commented Aug 10, 2018 • edited Loading

timabbott commented May 20, 2019

kkanahin commented Nov 8, 2016 •

edited

Loading

rishig commented Nov 8, 2016 •

edited

Loading

timabbott left a comment •

edited

Loading

zulipbot commented Nov 21, 2017 •

edited by synicalsyntax

Loading

timabbott commented Aug 10, 2018 •

edited

Loading