Question loading refactor #544
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
These methods alter the passed in object. We did not use the returned value, but it suggested that the returned object may be different. Remove it so that it is clear this is in-place modification.
For historic reasons (MongoDB!), when loading a question page we would load every single question attempt a user had ever made. We did this because we need both the attempts at the question parts on the question page, but also the attempts at any related questions. For registered users we can optimise this. Instead we now load the question attempts for the specific page in full; i.e. the complete QuestionValidationResponses. We need these for the bestAttempt attribute on the question page. For related questions, we can instead load LightweightQuestionValidationResponses since we don't need to deserialise all of the JSON to say whether they are complete or not. We can walk the content and only load the questions for the relevant question page IDs. Anonymous users have less question attempts and there are no equivalent methods for loading a subset of attempts (they still live in one JSON blob just like MongoDB), so we just load everything for them. The efficiency gains for highly active users are immense; for a user with thousands of question attempts, the page load time speedup is 10x. Most of the gain is from no longer deserialising irrelevant attempts, converting them back to strings for the ETag calculation.
Again, we don't need to load every question a user has ever made when loading a topic summary page.
This likely still isn't right, but it is better than before.
Again, we don't need to load every question attempt for registered users, only those relevant to the board.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #544 +/- ##
==========================================
- Coverage 26.48% 26.43% -0.05%
==========================================
Files 504 504
Lines 22896 22935 +39
Branches 2834 2839 +5
==========================================
Hits 6064 6064
- Misses 16281 16320 +39
Partials 551 551
☔ View full report in Codecov by Sentry. |
|
|
mlt47
reviewed
Jul 11, 2023
| @@ -505,6 +521,7 @@ public final Response getTopicSummaryPage(@Context final Request request, | |||
|
|
|||
| // Calculate the ETag on current live version of the content | |||
| // NOTE: Assumes that the latest version of the content is being used. | |||
| // Should this include the question attempts? | |||
There was a problem hiding this comment.
I would have thought so...
I think the reason this isn't a problem is that topic summary pages now don't have any related questions. They might have at some point although I think we moved to related gameboards quite quickly and then dropped them too.
The names now make the return types much clearer.
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.
For historic reasons (MongoDB! 😡), when loading a [question page/topic summary/gameboard] we would load every single question attempt a user had ever made. We did this on question pages because we need both the attempts at the question parts on the question page, but also the attempts at any related questions. The others just inherited the approach.
For registered users we can optimise this. Instead we now load the question attempts for the specific page in full; i.e. the complete
QuestionValidationResponses. We need these in full for the bestAttempt attribute on the question page.For related questions and gameboard items, we can instead load
LightweightQuestionValidationResponsessince we don't need to deserialise all of the JSON to say whether they are complete or not. We can walk the content and only load the questions for the relevant question page IDs.Anonymous users have less question attempts and there are no equivalent methods for loading a subset of attempts (they still live in one JSON blob just like MongoDB), so we just load everything for them.
There's my new favourite Java type:
Map<String, ? extends Map<String, ? extends List<? extends LightweightQuestionValidationResponse>>>all over the place, instead ofMap<String, Map<String, List<QuestionValidationResponse>>>or the lightweight equivalent. I had used this before in some places, but now it is everywhere. This allows us to say "we only need Lightweight validation responses" but accept full validation responses in some cases (like anonymous users for whom we only ever have full responses). It's a pain to read, but very powerful.The main place left, that this improved approach won't work for, but that still faces the main issue is the
generateTemporaryGameboardmethod; i.e. making a new random gameboard. We try to load questions you have not completed; but how to we do that without loading the questions in advance?