Only route to national overflow partners with capacity #6097
Conversation
|
Heroku app: https://gyr-review-app-6097-c47a8cf32ca3.herokuapp.com/ |
powersurge360
left a comment
powersurge360
left a comment
There was a problem hiding this comment.
Just a quick gutcheck on what may be a performance issue. If it's for sure a slim number I'm down to approve. If scale is much larger but we know it still scales well, also down to approve.
|
|
||
| def route_to_national_overflow_partner | ||
| national_overflow_locations = VitaPartner.where(national_overflow_location: true).order(Arel.sql('RANDOM()')) | ||
| national_overflow_locations = VitaPartner.with_capacity.where(national_overflow_location: true).order(Arel.sql('RANDOM()')) |
There was a problem hiding this comment.
Random in sql can have performance implications. What's the scale of the results here? If it's <50 probably fine. More than 50 and we maybe should dig in a little more. Note, 50 is pretty arbitrarily chosen but below that I'm pretty certain it won't be an issue.
There was a problem hiding this comment.
ryan said "I don’t think for the immediate few tax years" and "I can see a future where we want to aggressively expand the number of partners we serve, and that may mean bringing on more than 50 national overflow partners"
There was a problem hiding this comment.
@powersurge360 do you have any advice on doing this in a more efficient way?
There was a problem hiding this comment.
noting that that we have only have a 931 vita partners total and this article seems to suggest using rand is fine for tables under 10,000. Do you think with data dog monitoring our slow queries and presume-ably this number remaining pretty small for the next few years it would be okay to merge as is right now? Unless there is a really straightforward way to do this another way
There was a problem hiding this comment.
That sounds fine to me then! I knew that RANDOM had surprising performance issues in some circumstances but wasn't sure the scale. If you've looked into it and we're well beneath the scale it matters I'm happy! I'll go and approve. Appreciate the diligence.
|
Heroku app: https://gyr-review-app-6097-f5e166321812.herokuapp.com/ |
Link to pivotal/JIRA issue
Is PM acceptance required? (delete one)
Reminder: merge main into this branch and get green tests before merging to main
What was done?
How to test?