-
Notifications
You must be signed in to change notification settings - Fork 66
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
Schedule copay notification jobs in time intervals #12964
Conversation
@ScottyRJames just curious - do you have other team members that you could get to give their blessing before it gets to us? It's a process that we're slowly rolling out/requesting for all PRs, as it's difficult for us to give thorough PR reviews, especially when we don't have the full context or understanding of the business reqs. |
LGTM 👍 |
@ryan-mcneil Thanks for the heads up on this new process, @kjsuarez is another BE engineer on the debt resolution team and left a review earlier. Also added these files to our team within CODEOWNERSHIP to try to reduce the platform dependency for our team's managed code |
unique_statements.each_with_index do |statement, index| | ||
# For every BATCH_SIZE jobs, enqueue the next BATCH_SIZE amount of jobs JOB_INTERVAL seconds later | ||
CopayNotifications::NewStatementNotificationJob.perform_in( | ||
JOB_INTERVAL * (index / BATCH_SIZE), statement | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScottyRJames I just wanted to make sure this is what you intended. If JOB_INTERVAL and BATCH_SIZE were 90 and 100 respectively, then we'd be kicking off a new job every .9 seconds (not JOB_INTERVAL seconds), correct? Maybe I'm missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryan-mcneil It will kick off 100 new jobs every 90 seconds rather since ruby division defaults to the floor with integers.
For example, if one of the statements had index 127 it would be evaluated like 90 * (127 / 100)
-> 90 * 1
which perform_in
will take and schedule the job at 90 seconds later. So if unique_statements
were to be 500 statements long each of the slated times would be like
Indices | Start Time |
---|---|
0 - 99 | Time.now |
100 - 199 | Time.now + 90s |
200 - 299 | Time.now + 180s |
300 - 399 | Time.now + 270s |
400 - 499 | Time.now + 360s |
500 | Time.now + 450s |
If we wanted to go towards your thinking, it would look like like JOB_INTERVAL * (index / BATCH_SIZE).to_f
. I think by default Sidekiq checks for new jobs every 5 seconds so it wouldn't be to the exact millisecond that it's scheduled at. It's still workable if you think it would be better to stagger the jobs even more rather than spinning up a batch at a time but it would be more like 6-7 new jobs every 5 seconds vs a new job every .9 seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right. I wasn't considering the ruby division. Just wanted to make sure you considered it, and it looks like you're a step ahead of me 👌
following |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
* Schedule jobs using throttle technique * Fix time in comment * spread_interval -> job_interval * Make comment more clear * Add spec for batch processing * Add debt related workers to codeowners * Remove CODEOWNERS change
Summary
Related issue(s)
- department-of-veterans-affairs/va.gov-team#0000
https://github.com/department-of-veterans-affairs/devops/pull/13134
https://github.com/department-of-veterans-affairs/vsp-infra-application-manifests/pull/2010
Testing done
What areas of the site does it impact?
Sidekiq/copay notifications
Acceptance criteria
Requested Feedback
Any concern for spinning up batches of jobs every couple minutes?