Run background reports with Sidekiq, not fork #10615
Conversation
Forking worked in theory but crashed the browser in system specs. It also came with many other hurdles and isn't well known solution in the Rails community. Sidekiq can give us better control over execution limits as well.
| report_class, spree_current_user, params, format | ||
| ) | ||
| sleep 1 until job.done? |
There was a problem hiding this comment.
Just wondering why this is needed?
There was a problem hiding this comment.
Once the job is done, we want to render it here in the same web request. So we are waiting for the result. That's like the current behaviour. We display the report (or download) when it's ready.
It's planned to send a download link for longer running reports. Then we don't need to wait and can tell the user straight away that the report is processing. But we don't have that yet.
There was a problem hiding this comment.
Awesome, just a simple change to try this out!
Another consideration, which we discussed earlier, is that we might need to adjust the sidekiq workers/queues/priorities to help improve the responsiveness of the reports. But that would be after proving the concept.
| describe "Background processing" do | ||
| before do | ||
| Flipper.enable(:background_reports) | ||
| ActiveJob::Base.queue_adapter.perform_enqueued_jobs = true | ||
| end | ||
|
|
||
| it "can run the customers report" do | ||
| login_as_admin_and_visit admin_report_path( | ||
| report_type: :customers, report_subtype: :mailing_list | ||
| ) | ||
| click_button "Go" | ||
| expect(page).to have_content "EMAIL FIRST NAME" | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Because we're passing report parameters and chosen format to the job, it might be worth testing that these options have been used.
But I wouldn't suggest that prevents us from merging this, this is just a thought maybe for the next iteration.
|
Deployed to confirm ready for testing: |
|
Hey @mkllnk , I've ran the report as you've indicated before and after staging, and got the same data - all good. Also, after toggling the background_reports feature, I found all the reports to render correctly (I've added a note on the only exception): Orders And Distributors This looks good to go. Before merging, I'll still stage another PR, just to be sure about the failing report. Moving to ready to go! 🎉 (*) Additional Note - although not related to this PR
Any idea why? |
|
Merging: the Payment Report by Type still blows up for that user/date range, after staging another PR and disabling |
|
Nice testing, thank you. |
What? Why?
Forking worked in theory but crashed the browser in system specs. It also came with many other hurdles and isn't a well known solution in the Rails community. Sidekiq can give us better control over execution limits as well.
This is just a first iteration to test if it works in principle. If this is successful, we should continue with:
What should we test?
Before deploy:
After deploy:
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates