CSC/ECE 517 Fall 2021 - E2144. Refactor delayed mailer and scheduled task: Difference between revisions
Line 139: | Line 139: | ||
=== Sidekiq UI === | === Sidekiq UI === | ||
To see which messages were being enqueued and dequeued in Sidekig, we use the Sidekiq user interface, which can be accessed via the <code>/sidekiq</code> route of the application. | To see which messages were being enqueued and dequeued in Sidekig, we use the Sidekiq user interface, which can be accessed via the <code>/sidekiq</code> route of the application. | ||
[[File:Sidekiq-ui.png| | [[File:Sidekiq-ui.png|950px|The Sidekiq UI]] | ||
=== Code Cleanup === | === Code Cleanup === |
Revision as of 21:57, 17 October 2021
Background
This project revolves around the Sidekiq gem for asynchronous processing of email tasks. It has a queue system to hold and then process jobs. Sidekiq’s queue replaces DelayedMailer’s queue. The previous team that worked on this also created a method perform()
to gather email IDs of all participants in an assignment and send them an email reminder. Some test cases exist for this work.
Previous Setup
Initial work can be found here.
Code in the pull request was found to be reasonable and working. The method perform()
has:
Metric | Current Value | Limit |
---|---|---|
Cognitive Complexity | 15 | 5 |
Cyclomatic Complexity | 8 | 6 |
Assignment Branch Condition size | 22.93 | 15 |
Perceived complexity | 10 | 7 |
Previous pull request also has duplicated commit messages and the commits need to be squashed.
Files involved (some changed should not have been changed):
- app/controllers/assignments_controller.rb
- app/mailers/mail_worker.rb
- app/models/assignment_form.rb
- config/sidekiq.yml
- spec/models/assignment_form_spec.rb
- spec/sidekiq_mail_worker_spec.rb
- spec/spec_helper.rb
There are minor styling issues such as missing or trailing whitespaces, lines that are too long, extra empty line, unused arguments.
Problem Statement
This project revolves around the Sidekiq gem for asynchronous processing of email tasks. It has a queue system to hold and then process jobs. Sidekiq's queue replaces DelayedMailer’s queue. The previous team that worked on this also created a method perform() to gather email IDs of all participants in an assignment and send them an email reminder. Some test cases exist for this work. The project was partially merged.
What needs to be done: Feedback from the previous team (E1835) is: Code in pull request seems pretty reasonable. Some concerns wrt. naming functions, but otherwise reviewers seem to agree that the code is good.
Your pull request has many duplicated commit messages. Please try to squash
similar commits.
And using meaningful commit messages later.
You should commit changes to the DB schema (db/schema.rb
) only if you have created new DB migrations.
Please double check your code. If you did not aim to change the DB, please revert the DB schema changes. ***** Important, try not to change the DB.
You should not change rails_helper.rb
or spec_helper.rb
file; please revert these changes.
The previous team’s code needs a refactor in some places, though the actual function of it seems solid. It is very possible that in the three years since this code has been worked on, more mailer functionality has been added. There are other teams this semester working on mailer code as well. Try to keep your code as contained and simple as possible, so that merging these projects is possible.
Summary of Work Needed
After discussing with our project mentor and Dr. Gehringer, we determined the main tasks to be the following:
- Refactor the single Sidekiq worker into multiple workers
- Remove all code related to the
drop_one_member_topics
deadline type - Update and add unit tests according to the above changes
- Test all new worker classes to ensure Sidekiq jobs were being dequeued and run as expected
- Code cleanup
Refactor the single Sidekiq worker into multiple workers
Previously there was only a single worker class that handled all types of jobs. Its top-level method perform
looked like the following.
def perform(assignment_id, deadline_type, due_at) self.assignment_id = assignment_id self.deadline_type = deadline_type self.due_at = due_at assignment = Assignment.find(self.assignment_id) participant_mails = find_participant_emails if %w[drop_one_member_topics drop_outstanding_reviews compare_files_with_simicheck].include?(self.deadline_type) drop_one_member_topics if self.deadline_type == "drop_outstanding_reviews" && assignment.team_assignment drop_outstanding_reviews if self.deadline_type == "drop_outstanding_reviews" perform_simicheck_comparisons(self.assignment_id) if self.deadline_type == "compare_files_with_simicheck" else # Can we rename deadline_type(metareview) to "teammate review". If, yes then we donot need this if clause below! deadlineText = if self.deadline_type == "metareview" "teammate review" else self.deadline_type end email_reminder(participant_mails, deadlineText) unless participant_mails.empty? end end
The processing was different depending on the deadline_type
. The deadline_type
s in
%w[drop_one_member_topics drop_outstanding_reviews compare_files_with_simicheck]
did not lead to an email reminder being sent, but they were still handled in this MailWorker
class.
To make the code more manageable and easier to follow, it made sense to break up the responsibilities of this single MailWorker
class into multiple workers:
SimicheckWorker
DropOutstandingReviewsWorker
MailWorker
All three workers extended an "abstract class" called Worker
that contained the Sidekiq-related include
and the queue name to which all jobs would be sent.
For simplicity, we still had all new workers continue to use a single queue, which we renamed from mailers
to jobs
. A future improvement could divide these jobs to go to multiple different queues if needed.
Update and add unit tests according to the above changes
Test all new worker classes to ensure Sidekiq jobs were being dequeued and run as expected
Sidekiq setup
In order to test the Sidekiq workers, a few steps were needed to set up the workers and run Sidekiq (steps were obtained from this site):
- Initialize three new workers, each having one of the three new worker types:
rails g sidekiq:worker SimicheckWorker
rails g sidekiq:worker DropOutstandingReviewsWorker
rails g sidekiq:worker MailWorker
- In a terminal separate from the one in which the main rails application is running, run
bundle exec sidekiq
This Sidekiq-specific terminal will show logs and printed statements from the workers.
Sidekiq UI
To see which messages were being enqueued and dequeued in Sidekig, we use the Sidekiq user interface, which can be accessed via the /sidekiq
route of the application.