[#10964] Paginate enroll calls for large courses (FE) #10968

daongochieu2810 · 2021-02-16T15:36:18Z

Outline of Solution

Enable auto row resizing for input table
Break down the student enrollment request into requests of at most 100 students
Use concat from rxjs to wait and combine the responses of child requests
Works fine with 1000 students with a waiting time of 2-3 minutes on staging server. UX can be improved with a progress indicator

Demo on localhost with split-and-retry mechanism: here

moziliar

From the code construct, it looks alright!

Some pointers I noted down in the comments.

Suggestion: add a post-hook in your local repo (and put in .gitignore) to lint your code whenever you commit (just put npm run lint and ./gradlew lint) should be fine? Since linter will be run now or later.

Would be good to include a gif on how this enrollment process looks in both UI and network call (devtool) perspectives. Will have to measure the memory pressure but should not be a major concern in this issue. Of course, can do some testing in staging server too!

@rrtheonlyone would appreciate some feedback on #10943 as it would definitely help make this PR's UX a lot smoother. In the interim, if no big issue, perhaps you could merge that branch into this branch and start working on integrating the error display while that PR is undergoing final review.

Edit: can also merge this first before doing the error message optimisation. Just a suggestion.

.../pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.html

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

moziliar · 2021-02-16T17:01:39Z

Extra thought on the linearly scaled waiting time (4-5min for 1000 students): how can we accelerate this? Or is it a concern at all? @damithc

damithc · 2021-02-17T01:16:21Z

Extra thought on the linearly scaled waiting time (4-5min for 1000 students): how can we accelerate this? Or is it a concern at all? @damithc

Not at the moment. It's fine as long as the user sees regular progress.

moziliar

The problem of misalignment still persists. Please look at the 300+ LoC. This will most likely lead to lint failure anyways (not sure why the CI is not running for this PR to master)

moziliar

LGTM now. But pending discussion on whether to increase the batch size and split on demand.

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

moziliar

A question for discussion: if you use chunk in many places, should you already define an interface for it to make the code look cleaner? Not a suggestion but a food for thought.

Could you snip a gif showcasing how it would work? I think a chunk size of 200 would be good demo for timeout and split.

Keep up the good work!

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

moziliar

Impressive 💯! I like the code construct in the current phase :)

Can you simulate the error occasion and see how the request is broken down into two halves? I believe this can be done by adding some lines to make some chunks larger.

Also, concerning @rrtheonlyone 's worry about quick succession and eventual consistency of the datastore we use, should we pad some interval in between enrolments to achieve a best effort consistency preservation? On the other hand, I did have the thought of using FE to perform pre-flight check on the to-enrol students. That is for another time. Given the latency in local server, I suspect a 1s backoff can allow the datastore instance to reach quorum most of the time.

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

moziliar · 2021-02-26T09:08:31Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

+    (resp: Students) => {
+      if (resp.students.length !== 0) {
+        this.loadExistingStudentsData(existingStudentsHOTInstance, resp.students);
+      } else {
+        // Shows a message if there are no existing students. Panel would not be expanded.
+        this.isExistingStudentsPresent = false;
+        this.isExistingStudentsPanelCollapsed = !this.isExistingStudentsPanelCollapsed; // Collapse the panel again
+      }
+    }, (resp: ErrorMessageOutput) => {


Why the change of indentation here?

rrtheonlyone

Thanks for the work put in here 👍

To me this is not the way forward for this... just because we got away with it in downloading session results, doesn't mean it always works.

Reasons why I feel this solution is too naive:

the number you break up into (200?) is a magic number and just a hopeful toss at the moment (unless you have stress tested on this)
the correction mechanism is adding unnecessary complexity to the code base (why split in half and try API again?)
datastore is eventually consistent (we have no control over this and we can't wait it out and hope we get correct data) => this adds more problems since we may end up with wrong data in the db

My stance on this is that we should aim to optimize the logic on the backend - that is more firm and will stand the test of time. Of course, feel free to convince me otherwise.

There must be some way to come up with a new execution logic for validation + creation. From how we interact with the database and how we do the validation in the logic layer.

First step, I feel is to separate enrolling new students from updating old students (or be more clever about how we figure out if a student is already there). If that can be done, there are def ways we can optimize the validation and creation logic to easily handle <1000 students. We can do a projection query to retrieve the needed data / we can do a batch create for instance.

My intuition tells me that this batching with retry mechanism on the frontend like this is unnecessarily complex - we are operating in the scale of 100s (its not like there is millions of datapoints). However, I may be wrong - def warrants some discussion.

moziliar · 2021-02-27T10:48:23Z

Undeniably, the current logic is too complex atm.

Below are some of my opinions:

A common approach to improving such workflow (making multiple user calls simpler) is usually just automate what the user does: performing the same action (enrolling students in batches) as the user would, but through the help of frontend code.
The doubt about eventual consistency and indeterministic processing time brings about the split and retry, which is actually safe as the enrolment is actually an upsert action which is idempotent.

Improving the workflow won't help if the bulk of the workload is in IO bound operation, likened to Amdahl's law. Paralleling the individual enrolment might help but depends on the instance resource. However, if batching student entity creation helps in DB later, it's worth investigating. To this, I'd like to check the scalability of the solution still. Although I'm sure we won't have to enrol a University worth of students into a single course.

rrtheonlyone · 2021-02-27T11:56:26Z

I am not sure what you are trying to say above. I am bouncing the idea that we can try achieve this entire operation by making 1/2 API calls with no batching (with backend optimization).

The doubt about eventual consistency and indeterministic processing time brings about the split and retry, which is actually safe as the enrolment is actually an upsert action which is idempotent.

No - split and retry is only triggered upon error. What if there is no error but a succesful enrollment after validating with stale data?

I am suspecting that the eventual consistency issue is going to be worsened by the repeated API calls since it is only increases your chances of getting stale data from the database. I do not know why this is "actually safe" as you point out.

Even the current production version has this problem - just that the probability of it happening is almost zero since the instructor will take some time to break up and re-upload. (can you help me test and say the same for your solution here?)

A common approach to improving such workflow (making multiple user calls simpler) is usually just automate what the user does: performing the same action (enrolling students in batches) as the user would, but through the help of frontend code.

This is indeed what this PR is doing. I have already addressed my concerns to this idea above. We need to be clear that this is the best way forward for this. (e.g. show that no amount of backend optimisation can help).

Improving the workflow won't help if the bulk of the workload is in IO bound operation, likened to Amdahl's law.

Yes, that's right - why not optimize on the IO operations then? Something like getting only a subset of data (projections) and making a small amount of calls to enroll (batch operations). I am not talking about parallelising here haha.

moziliar · 2021-02-27T14:05:21Z

We should look into BE optimization for sure. The current entity-by-entity enrolment logic can definitely be optimized (unsure about the benefit without concrete stress test result: say if we shave 70% of the latency and allow 250 students enrolment instead of 100, is this scalable enough? This might be my naive thought, will test it out)

Just bouncing some ideas against eventual consistency issue: would enforcing strong consistency at objectify level help (like ofy().consistency(ReadPolicy.STRONG))? Or for a single enrolment workflow (given the condition that no other instructors are performing enrolment with the exact same data set at the exact same time), would validating the data in the frontend minimise the problem?

rrtheonlyone

Great work! Code looks much better.

Here is another odd behaviour:

Suppose we have for instance 200 students and there is an error in the 200th student that only the backend can catch. When we click enroll, we get back an error message about the 200th student. However, we don't see any enrollment panel about the successful enrollment of students 100-150. This is def tricky but we should be clear to the user about which students got enrolled - right now a user might think the whole batch failed and might get a surprise later on.

rrtheonlyone · 2021-03-21T00:38:42Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

+    const noInvalidRowsBefore: number = this.invalidRowsIndex.size;
+
+    Array.from(studentEnrollRequests.keys()).forEach((key: number) => {
+      const request: StudentEnrollRequest | undefined = studentEnrollRequests.get(key);


when would this be undefined?

this would not be undefined, but enforcing non-null is not permitted and so putting studentEnrollRequests.get(key)!! will not pass lint

rrtheonlyone · 2021-03-21T00:40:37Z

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

+
+      if (teamSectionMap.get(request.team) !== request.section) {
+        this.invalidRowsIndex.add(key);
+        const firstIndex: number | undefined = teamIndexMap.get(request.team);


same here - why undefined (also check for the others)

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts

daongochieu2810 · 2021-03-21T07:28:50Z

@rrtheonlyone Hi I could not reproduce the scenario you mentioned. Here I try to enroll 200 students with the last one having an incorrect email which can only be caught by backend, but the panel still shows correct enrollment results

rrtheonlyone · 2021-03-21T11:57:34Z

@daongochieu2810 Try put a duplicate team for the last student instead (duplicate team that can only be caught by the backend). I just tried with 51 students (where the 51st student has the error) and this is what I see:

I verified that the first 50 have actually been uploaded.

…hieu2810/teammates into hieu/enroll-large-course

daongochieu2810 · 2021-03-21T14:23:37Z

@rrtheonlyone The result panel is populated if an error happens

rrtheonlyone · 2021-03-21T15:19:06Z

Yup better now - however the enrollErrorMessage now becomes overridden by GENERAL_ERROR_MESSAGE. We should be seeing the error message corresponding to duplicate team that is returned by the backend.

daongochieu2810 · 2021-03-21T15:43:28Z

I solved that by setting error message after populating the result panels

rrtheonlyone

LGTM 👍

madanalogy · 2021-03-29T05:22:08Z

@daongochieu2810 can you resolve the conflicts and update me after

…hieu2810/teammates into hieu/enroll-large-course

daongochieu2810 · 2021-03-29T07:16:52Z

@madanalogy I have resolved the conflicts

madanalogy · 2021-03-29T07:32:39Z

Could you help to fix the failing component tests as well? Think you just need to update a snapshot as in #11051

Paginate enroll calls

7f767c9

daongochieu2810 requested review from moziliar and rrtheonlyone February 16, 2021 15:58

moziliar requested changes Feb 16, 2021

View reviewed changes

moziliar added the s.Ongoing The PR is being worked on by the author(s) label Feb 16, 2021

Adjust code style

d678f38

daongochieu2810 requested a review from moziliar February 17, 2021 06:16

moziliar requested changes Feb 17, 2021

View reviewed changes

Reformat html file for instructor enroll page

47b8c9d

daongochieu2810 requested a review from moziliar February 17, 2021 14:45

moziliar reviewed Feb 18, 2021

View reviewed changes

...pp/pages-instructor/instructor-course-enroll-page/instructor-course-enroll-page.component.ts Show resolved Hide resolved

moziliar added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 18, 2021

daongochieu2810 added 2 commits February 21, 2021 13:41

Increase number of students per chunk

f405b3d

Add mechanism to break long request smaller if timeout

4bf44cc

daongochieu2810 requested a review from moziliar February 21, 2021 16:56

moziliar requested changes Feb 22, 2021

View reviewed changes

daongochieu2810 force-pushed the hieu/enroll-large-course branch from 6e07cc5 to 7fed62b Compare February 22, 2021 17:22

Refactor code and update test

57c021f

daongochieu2810 force-pushed the hieu/enroll-large-course branch from 7fed62b to 57c021f Compare February 22, 2021 17:49

daongochieu2810 requested a review from moziliar February 22, 2021 18:02

moziliar requested changes Feb 26, 2021

View reviewed changes

daongochieu2810 and others added 2 commits February 26, 2021 17:50

Add delay of 1s between paginated calls

aaeb85c

Merge branch 'master' into hieu/enroll-large-course

a4e99e8

rrtheonlyone requested changes Feb 27, 2021

View reviewed changes

rrtheonlyone requested changes Mar 21, 2021

View reviewed changes

daongochieu2810 requested a review from rrtheonlyone March 21, 2021 10:05

daongochieu2810 added 2 commits March 21, 2021 22:19

Show enroll results when error happens

753ae21

Merge branch 'hieu/enroll-large-course' of https://github.com/daongoc…

c69913f

…hieu2810/teammates into hieu/enroll-large-course

Set error message after populating result panels

5ead34c

rrtheonlyone approved these changes Mar 21, 2021

View reviewed changes

daongochieu2810 added s.FinalReview The PR is ready for final review and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 22, 2021

Merge branch 'master' into hieu/enroll-large-course

827c9d1

daongochieu2810 mentioned this pull request Mar 22, 2021

[#10936] Show explicit enroll errors #10943

Merged

madanalogy 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 23, 2021

madanalogy assigned rrtheonlyone, moziliar and hcwong Mar 23, 2021

madanalogy added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Mar 23, 2021

madanalogy added this to the V7.13.0 milestone Mar 23, 2021

daongochieu2810 added 2 commits March 29, 2021 15:15

Resolve conflicts

35cc44d

Merge branch 'hieu/enroll-large-course' of https://github.com/daongoc…

01a7787

…hieu2810/teammates into hieu/enroll-large-course

daongochieu2810 and others added 2 commits March 29, 2021 16:01

Fix lint

a8335a2

Merge branch 'master' into hieu/enroll-large-course

badde77

madanalogy merged commit 72bafbf into TEAMMATES:master Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10964] Paginate enroll calls for large courses (FE) #10968

[#10964] Paginate enroll calls for large courses (FE) #10968

daongochieu2810 commented Feb 16, 2021 •

edited

Loading

moziliar left a comment •

edited

Loading

moziliar commented Feb 16, 2021

damithc commented Feb 17, 2021

moziliar left a comment •

edited

Loading

moziliar left a comment

moziliar left a comment

moziliar left a comment

moziliar Feb 26, 2021

moziliar Mar 17, 2021

rrtheonlyone left a comment •

edited

Loading

moziliar commented Feb 27, 2021

rrtheonlyone commented Feb 27, 2021

moziliar commented Feb 27, 2021

rrtheonlyone left a comment

rrtheonlyone Mar 21, 2021

daongochieu2810 Mar 21, 2021

rrtheonlyone Mar 21, 2021

daongochieu2810 commented Mar 21, 2021

rrtheonlyone commented Mar 21, 2021 •

edited

Loading

daongochieu2810 commented Mar 21, 2021 •

edited

Loading

rrtheonlyone commented Mar 21, 2021 •

edited

Loading

daongochieu2810 commented Mar 21, 2021

rrtheonlyone left a comment

madanalogy commented Mar 29, 2021

daongochieu2810 commented Mar 29, 2021

madanalogy commented Mar 29, 2021

[#10964] Paginate enroll calls for large courses (FE) #10968

[#10964] Paginate enroll calls for large courses (FE) #10968

Conversation

daongochieu2810 commented Feb 16, 2021 • edited Loading

moziliar left a comment • edited Loading

Choose a reason for hiding this comment

moziliar commented Feb 16, 2021

damithc commented Feb 17, 2021

moziliar left a comment • edited Loading

Choose a reason for hiding this comment

moziliar left a comment

Choose a reason for hiding this comment

moziliar left a comment

Choose a reason for hiding this comment

moziliar left a comment

Choose a reason for hiding this comment

moziliar Feb 26, 2021

Choose a reason for hiding this comment

moziliar Mar 17, 2021

Choose a reason for hiding this comment

rrtheonlyone left a comment • edited Loading

Choose a reason for hiding this comment

moziliar commented Feb 27, 2021

rrtheonlyone commented Feb 27, 2021

moziliar commented Feb 27, 2021

rrtheonlyone left a comment

Choose a reason for hiding this comment

rrtheonlyone Mar 21, 2021

Choose a reason for hiding this comment

daongochieu2810 Mar 21, 2021

Choose a reason for hiding this comment

rrtheonlyone Mar 21, 2021

Choose a reason for hiding this comment

daongochieu2810 commented Mar 21, 2021

rrtheonlyone commented Mar 21, 2021 • edited Loading

daongochieu2810 commented Mar 21, 2021 • edited Loading

rrtheonlyone commented Mar 21, 2021 • edited Loading

daongochieu2810 commented Mar 21, 2021

rrtheonlyone left a comment

Choose a reason for hiding this comment

madanalogy commented Mar 29, 2021

daongochieu2810 commented Mar 29, 2021

madanalogy commented Mar 29, 2021

daongochieu2810 commented Feb 16, 2021 •

edited

Loading

moziliar left a comment •

edited

Loading

moziliar left a comment •

edited

Loading

rrtheonlyone left a comment •

edited

Loading

rrtheonlyone commented Mar 21, 2021 •

edited

Loading

daongochieu2810 commented Mar 21, 2021 •

edited

Loading

rrtheonlyone commented Mar 21, 2021 •

edited

Loading