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

Asset modified cleanup rfc document #832

Merged
merged 7 commits into from
May 3, 2024

Conversation

JackLewis-digirati
Copy link
Contributor

@JackLewis-digirati JackLewis-digirati commented Apr 26, 2024

This PR adds an RFC discussing #430

@JackLewis-digirati JackLewis-digirati changed the title Adding initial asset modified cleanup rfc Asset modified cleanup rfc document May 1, 2024
@JackLewis-digirati JackLewis-digirati marked this pull request as ready for review May 1, 2024 09:30
@JackLewis-digirati JackLewis-digirati requested a review from a team as a code owner May 1, 2024 09:30
Copy link
Member

@donaldgray donaldgray left a comment

Choose a reason for hiding this comment

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

It's not explicitly mentioned but cleanup logic will need to take into account CustomerOriginStrategy, policy for image (use-original/default) and other available channels (i.e. it can't just look at the delivery-channels individually).

A thought when reading: How would this work during period of mass reingestion? ie if 10K..100K..1M..10M items are queued - items added last in the queue could take hours/days to be picked up by Engine. The max value for delay_seconds is 15mins so everything will be first picked up pretty quickly - assuming Engine is still in flight they wouldn't be acted on. As mentioned we can use visibility_timeout (max 12 hours) to ensure it's not picked up again for a while but there are limits on the number of in-flight messages. The limit is large and we're using long-polling which means we wouldn't receive an error but would receive empty responses until there are less messages in-flight. Do we need to bear that in mind for this doc? Even if it's a backup solution to manually disable asset-cleanup if we know a mass ingest will happen.

General comment, there's inconsistent capitalisation + use of full stops

docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
@donaldgray donaldgray self-requested a review May 3, 2024 08:35
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
docs/rfcs/017-asset-modified-cleanup.md Outdated Show resolved Hide resolved
@JackLewis-digirati JackLewis-digirati merged commit 7ec889d into develop May 3, 2024
@JackLewis-digirati JackLewis-digirati deleted the feature/assetModifiedRfc branch May 3, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants