Fix DB crash due to timeout #1031
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Improves read performance.
* Use subquery instead of join for answers * Split main model query and view model generation * Split queries into 3 * Use ORM where possible
Fixes compatibilty with new function signature.
YaleChen299
approved these changes
Nov 7, 2023
| |> Enum.map(fn c -> String.to_atom(c) end) | ||
| |> Enum.zip(&1) | ||
| |> Enum.into(%{})) | ||
| ) |
There was a problem hiding this comment.
do we put a default case here? in case of error
There was a problem hiding this comment.
I didn't think it was necessary as I don't think the SQL will ever error (unless the DB server itself is down, in which case it would give the same error messages as the ones we have been having recently).
What do you think?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our grading summary uses a long, raw SQL query to generate the JSON directly, bypassing our ORM. This causes issues when we have hundreds of thousands of answers to query.
Lots of details are glossed over in this PR as they have already been discussed internally.
Changes:
While request-response delay is only slightly improved, DB query time is improved much more significantly, as a lot of the view generation logic is done in the backend layer now. The DB only provides the bare minimum amount of data necessary to generate this view.
Tested and JSON output is of the exact same structure compared to the old query.
CAVEAT
This PR will break Avenger backlog emails. Not just because I removed the "ungraded only" functionality (which was only added in #932 and used in emails), but while compile errors are fixed to match the new function signature, it will most definitely result in a runtime error (as the email templates are not updated to reflect the new structure.
The reason I think it's fine is:
Email notifications are not used anywhere yet
I really think Add Avenger Backlog Notifications #932 is the wrong approach to generate the emails:
all_submissions_by_grader_for_indexgenerates a view (back then was still the JSON string), which is then converted back to a "model" usingJason.decode, which is then further manipulated, and the final view is generated using the email template.My proposal would be to create a separate function entirely for this functionality, which would optimise everything as well. But that is out of scope of this, and will come in a follow-up PR instead.