Skip to content
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

Add retention configuration for activity, revisions, and notifications #20683

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

azrikahar
Copy link
Contributor

@azrikahar azrikahar commented Dec 8, 2023

Context: this PR carries over changes from #18105, but with modifications to the truncate logic and additional checks & migration.

Scope

What's changed:

  • Introduces several environment variables to configure retention periods
    • For notifications, only clean up archived notifications
    • For activity, currently clean up all except comments & content version saves
    • opt to log error and return early if they are misconfigured, rather than throwing an error to avoid blocking those requests (even if this means the clean up doesn't run)
  • Add migration to add index on directus_revisions's item field

Potential Risks / Drawbacks

  • The new defaults for retention may be a breaking change for existing setups

Review Notes / Questions

  • In regards to the exception for activity regarding comments & content versioning saves, perhaps it's better for it to be configurable? But wondering should that specific thing be an env variable or configurable via project settings.
  • The migration currently adds an index, but technically existing users may already have an index in place. Would there be a way to check whether there's an existing index? I wasn't able to find how would we be able to do said check via knex (or maybe we'll have to do it via setting up our own helpers?)
  • Should we have a strategy to export out those to-be-deleted rows before they're actually deleted for archival purpose? (or should that be a separate PR)

Todo

  • Export to-be-deleted records to storage before truncating
  • Update migration file name before merging

Fixes #17894

Copy link

changeset-bot bot commented Dec 8, 2023

🦋 Changeset detected

Latest commit: 2d1cd91

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/api Minor
docs Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@azrikahar azrikahar marked this pull request as draft December 8, 2023 16:27
Copy link
Member

@joselcvarela joselcvarela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should export the records to the current storage as CSVs.
This will introduce an unexpected behaviour to the current projects so, if we have, at least, the exported records on storage, these projects would still be able to access those records.
The problem is that some project have millions of records on revisions, so exporting those millions of records may be a challenge. Any thoughts on that? 🤔

api/src/env.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🆕 Needs Triage
Development

Successfully merging this pull request may close these issues.

Performance issues with revisions and activity
4 participants