Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Background processing #2261

Closed
wants to merge 3 commits into from
Closed

Background processing #2261

wants to merge 3 commits into from

Conversation

morgoth
Copy link
Contributor

@morgoth morgoth commented Jul 23, 2016

Very basic implementation for background processing support using ActvieJob.

It requires more work, but I would like to start collecting your feedback about implementation and possible features.

I tested it with file and s3 storages locally.

Questions:

  • I think it make sense to add db column where we can mark that file is being processed () - do you think boolean :attachment_name:_processing_in_background is fine? done
  • To force processing in background one have to set array of styles in process_in_background options, but also set only_processing to fake [:none], as empty array processes all by default. I would like to solve it some better way. How it should behave?

[ref #2260]

rebuild_model styles: {thumb: "100x100"},
only_process: [:none],
process_in_background: [:thumb]
@file = File.new(fixture_file("5k.png"), 'rb')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

it "processes styles marked for background processing" do
file = File.new(fixture_file("5k.png"), "rb")
FileUtils.cp(file, "tmp/public/system/dummies/avatars/000/000/001/original/5k.png")
rebuild_model styles: {thumb: "100x100"},

Choose a reason for hiding this comment

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

Space inside { missing.
Space inside } missing.

require "active_job"
rescue LoadError
raise LoadError,
"To use background processing you have to include 'active_job' in load path"

Choose a reason for hiding this comment

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

Align the parameters of a method call if they span more than one line.

@morgoth
Copy link
Contributor Author

morgoth commented Jul 23, 2016

I'm also wondering how configuration for future features should look like, in example:
has_attached_file :avatar, process_in_background: [:thumb], background_job_queue_name: "paperclip, ... or maybe group them all under one key?
has_attached_file :avatar, process_in_background: {styles: [:thumb], queue_name: "paperclip"}}

only_process: [:none],
process_in_background: [:thumb]

dummy = Dummy.create!(avatar_file_name: "5k.png", avatar_processing_in_background: true)

Choose a reason for hiding this comment

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

Line is too long. [92/80]

@andreiglingeanu
Copy link

andreiglingeanu commented Jul 24, 2016

@morgoth Let me know if I can help with something. Little bit of testing, anything. I'll be in touch.

Really want to see this thing in paperclip.

I'm on using this, at least for now.

@ScotterC
Copy link
Contributor

ScotterC commented Aug 1, 2016

As a maintainer of DelayedPaperclip, I'd love for this to be in Paperclip core. Much less hacky and easier to maintain in the future.

@morgoth
Copy link
Contributor Author

morgoth commented Aug 18, 2016

@tute Do you think we can move it forward?
I think there are 2 blockers:
#2261 (comment)
#2261 (comment)

@bdewater
Copy link
Contributor

How would callbacks work if this is integrated? I have a potential use case that looks quite a lot like the one described here: jrgifford/delayed_paperclip#176

@morgoth morgoth changed the title POC for background processing Background processing Dec 17, 2016
@Tonkpils
Copy link

any update on this PR?

@sidraval
Copy link
Contributor

We are in the process of deprecating paperclip, and so are closing PRs that aren't immediate bug fixes or documentation updates. Thanks for your contribution, we regret not tending to it!

@sidraval sidraval closed this Apr 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants