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

Update draft status of existing attachment assets #3769

Merged
merged 2 commits into from Feb 15, 2018

Conversation

@floehopper
Copy link
Contributor

floehopper commented Feb 15, 2018

This is based on the changes in #3768 which keeps the draft status of assets for attachments up-to-date on an on-going basis, including new ones. This PR adds a Rake task to allow us to update the draft status of assets for all existing attachments.

Only the last two commits are unique to this branch - I plan to rebase this branch against the branch in #3768 before merging this PR.

The new asset_manager:attachments:update_draft_status Rake task iterates over a range of attachments and invokes ServiceListeners::AttachmentDraftStatusUpdater#update! to queue up jobs to update the draft status of the corresponding assets in Asset Manager. Note that I've tweaked this AttachmentDraftStatusUpdater class slightly to allow the Rake task to specify the (lower priority) asset_migration Sidekiq queue in order to avoid blocking up business-as-usual activity in the app.

Like some of the other asset migration Rake tasks, this new Rake task allows a range of attachment IDs to be specified so we can process the attachments in manageable batches.

We're probably going to need to do something similar for the following issues:

I wonder what the rate-limiting step was when we migrated the attachment files, because if running the new Rake task for all attachments is going to take a similar amount of time, we might want to consider either (a) combining the work from all the above issues into a single migration job; or (b) investigating increasing the throughput of the Whitehall/Asset Manager queueing mechanism.

@chrisroos, @chrislo: Do you have any thoughts on the latter?

Copy link
Contributor

chrisroos left a comment

Looks good to me, @floehopper.

floehopper added 2 commits Feb 15, 2018
I want to re-use this class in a Rake task for updating existing
attachments and it will need to run at a lower priority, specifically
using the `asset_migration` queue.
@floehopper

This comment has been minimized.

Copy link
Contributor Author

floehopper commented Feb 15, 2018

Now that #3768 has been merged, I'm going to rebase this against master and force-push in preparation for merging.

@floehopper floehopper force-pushed the update-draft-status-of-existing-attachment-assets branch from 80cf141 to 2a9af58 Feb 15, 2018
@floehopper floehopper merged commit 64bb168 into master Feb 15, 2018
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
@floehopper floehopper deleted the update-draft-status-of-existing-attachment-assets branch Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.