Skip to content
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

[#10959] Paginate frontend call to webapi for large course #10960

Conversation

moziliar
Copy link
Contributor

@moziliar moziliar commented Feb 13, 2021

Closes #10959

Outline of Solution

Break down result download to per question level. Might need to experiment with the threshold to make granular download for smaller courses (OOM issue?)

From the current experiment, since the responses per question fetched are in different servlet, the memory pressure is minimal, possibly due to that GC can safely reclaim the servlet memory. This is better for even smaller courses that might result in memory spike for download (observed for a decently sized course where the memory shot up to close to the maximum)

Edit

Able to download course far larger than the previous threshold (roughly 600 students, 5 man team and 25 questions) in a synchronous manner, which incur long waiting time that is acceptable.

image

Total memory pressure throughout the download is expected to remain low as compared to the previous implementation where all questions are fetched and bundled together before returning. GC doesn't seem to be working after the servlets return, whose possible cause could be due to memory madvise memory kipnap.

image

Additionally, it is reasonable to believe that the main memory consumption comes from the DB read object. As such, optimizing memory consumption might be hard. Within 12 calls (from the profiler) to fetch 1 question from a 700 students 5 man team course, the heap allocated for getSessionResultsForUser reaches 65MB, which averages to about 5MB for a 400KB output. This is reasonable at the moment

image


Added a modal that informs the user the progress of the download. This is done in a modal form as the page is effectively frozen while downloading. If the user exits the modal by either closing it or clicking "Abort" button, the download will stop and no csv file will be generated.

Update

New download modal

108107680-86128f00-70ca-11eb-8f96-ffba0a988bb8

@moziliar moziliar added the s.ToReview The PR is waiting for review(s) label Feb 15, 2021
@teikjun
Copy link
Member

teikjun commented Feb 15, 2021

I didn't notice this earlier, but the "Download Results" button can be accessed from the "Home" and "Sessions" tab via the session table as well. When accessed this way, the download is not processed in batches, so it may TLE like before. I'm still trying to resolve this, I'll merge it together with the frontend changes later.

image

At the moment, instructor-session-base-page doesn't load any question-related information. I'm not sure which of the following options is preferable, will appreciate any suggestions for how to handle this issue.

  1. Load all the feedbackQuestionId for the relevant session after user hits "Download Results". Then, break down the download to question-level.
    (Rationale: We need feedbackQuestionId of the session in order to break down the download to question-level. But, we cannot load feedbackQuestionId of all the sessions on the page at the start - the page would load very slowly if there are many sessions)
  2. Remove the "Download Results" button from session table. Users can download through "View Results" > "Download Results".
  3. Leave it as it is - "Download Results" on session table may TLE, while "Download Results" on session result page will process the download in batches.

On the UI side, I'll position the progress bar on top of a modal, so it displays correctly on both pages.

@moziliar
Copy link
Contributor Author

@teikjun Sorry that this PR did not tackle that issue...
I believe solution 1 is more probable. So the rationale for this is that the workflow either consists of

  • instructor click download result from session base -> FE fetches all questionIds -> FE downloads with paginated calls
  • instructor click view result -> FE fetches all questionIds (and the rest...) -> instructor click download results -> FE downloads with paginated calls
    Comparing the above, the former seems more probable as a smoother workflow without additional overhead (or less) in latency. I would encourage that.

@rrtheonlyone rrtheonlyone added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Feb 21, 2021
teikjun and others added 2 commits February 23, 2021 17:31
…ebapi-for-large-course

Add transparent overlay  for loading modal
Copy link
Contributor

@rrtheonlyone rrtheonlyone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 💯 - I think this PR does a pretty good job of fixing the issues faced in the past when downloading session results for large courses.

The change is pretty small since we are just leveraging on existing functionality. Good to see some stress testing done above.

Regarding the user experience:

  • We should abort the progress bar automatically upon failure. Right now the progress bar just freezes and we see a toast.
  • There is an empty file committed in the PR (please remove that)
  • When we click somewhere on the screen right now, the progress bar disappears but the download still continues to take place - we should not close progress bar modal if user clicks outside.

@@ -66,4 +66,14 @@ export class SimpleModalService {
};
return this.open(header, type, content, modalOptions);
}

openDownloadModal(header: string, type: SimpleModalType,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rephrase this to openLoadingModal? Since this is a generic service that can be reused for other functions.

@teikjun
Copy link
Member

teikjun commented Feb 27, 2021

Thanks for the reviews! The requested changes have been made in moziliar#23, and will be merged into this main PR soon. I have two concerns:

1. Clicking outside the screen:

The loading modal (containing the progress bar) isn't supposed to disappear when user clicks outside the screen - there is a transparent overlay (positioned behind the loading modal and in front of the content) that is intended to prevent that. I'm not able to reproduce the bug so far on Chrome/Firefox. @rrtheonlyone, can I ask what browser are you using? I would like to further investigate this bug.

2. Empty files:

I noticed that there are multiple empty .scss files in the codebase. Should I open another issue/PR to remove the rest of them?

@rrtheonlyone
Copy link
Contributor

I am using chrome version 88.0.4324.192 (Official Build) (x86_64) on macOS. I just tried and it seems I can click outside and close the modal:

captured_TM

Regarding empty files, small issue - I think can leave it then.

@teikjun
Copy link
Member

teikjun commented Feb 28, 2021

Thanks @rrtheonlyone for the information! The bug has been resolved in moziliar#23.
We'll review the changes internally and then request a final review from you.

rrtheonlyone and others added 2 commits March 1, 2021 23:52
Copy link
Contributor

@rrtheonlyone rrtheonlyone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

UI looks good now! Just one minor thing.. @teikjun could you also abort the download when the 'X' button is pressed in the modal? Right now even after you press the 'X' - the download continues :P Once that is fixed, we can merge in.

@teikjun
Copy link
Member

teikjun commented Mar 6, 2021

The requested change has been made in moziliar#24. Good to go after getting approval from @moziliar 👍

moziliar and others added 2 commits March 9, 2021 09:53
@rrtheonlyone
Copy link
Contributor

Looks good :)

@rrtheonlyone rrtheonlyone added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Mar 13, 2021
@rrtheonlyone rrtheonlyone added this to the V7.12.0 milestone Mar 13, 2021
@rrtheonlyone rrtheonlyone merged commit 39dae8c into TEAMMATES:master Mar 13, 2021
@madanalogy madanalogy added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Mar 14, 2021
@rrtheonlyone rrtheonlyone mentioned this pull request Mar 22, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable large course response download
6 participants