Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

ssciolla · 2022-02-03T19:18:46Z

The PR aims to resolve #326.

(I'm pretty sure this does the trick, and it kinda feels like magic 🪄 .)

Resource(s):

ccm_web/package.json

pushyamig · 2022-02-04T17:47:01Z

I will review this some time in late afternoon.

pushyamig · 2022-02-04T21:11:57Z

I did some testing with 400 user enrolling 4 times and in all the time i saw the success. here is the report from that. I might do testing on Monday.

ccm_web/server/src/api/api.utils.ts

pushyamig · 2022-02-09T15:57:27Z

I will review some more by end of day

ssciolla · 2022-02-09T21:23:41Z

ccm_web/server/src/api/api.admin.handler.ts

+    const coursesApiPromises = createLimitedPromises<CanvasCourse[] | APIErrorData>(
+      accountIds.map(a => async () => await this.getAccountCourses(a, queryParams))
+    )


We probably don't have to limit here, since we're just making one (albeit potentially rather large paginated request) for each parent account the user has. It can't be that many, even if they're a sub account admin in several accounts. Especially if we're at 20 for a limit, this won't make much of a difference.

Good thinking on wrapping the concurrency code to Getting courses from an account, I was bit worried if user search by course name Bio then it might fetch lot of courses and will end up with rate limiting errors, this should take care it

Just to clarify, this is limiting the number of promises for calls of this.getAccountCourses, so if there were three accounts that someone was a sub-account admin for, that would be three promises. The @kth/canvas-api library is doing the paging, so that may actually involve multiple calls behind the scenes (probably in sequence?), but for us it's just one promise. Does that make sense? That's why I was saying it was minimal improvement here.

I elected to remove the limiting here. I think it's highly unlikely that anyone would be an admin in >20 sub-accounts. We can reassess if we ever see an edge case scenario.

ccm_web/server/src/api/api.course.handler.ts

ssciolla · 2022-02-09T21:28:41Z

ccm_web/server/src/api/api.service.ts

+    const apiPromises = createLimitedPromises<CanvasCourseSectionBase | APIErrorData>(
+      sectionIds.map(si => async () => {
+        const sectionHandler = new SectionApiHandler(requestor, si)
+        return await sectionHandler.unmergeSection()
+      })
+    )


See https://github.com/tl-its-umich-edu/canvas-course-manager-next/pull/327/files#r803102325, same logic applies.

Why unmerge has to be wrapped with the concurrency call? since we mostly ever going to do 1 section unmerge at a time

That's fine, I can remove it if you want, I was just looking for feedback on which to remove it from.

Theoretically, someone could do up to 250 using the API, but not likely to ever happen.

Since UI doesn't support bulk unmerge and only one at a time, I believe we can remove it

So you're not worried about direct API calls? This is the DTO for both merge and unmerge: https://github.com/tl-its-umich-edu/canvas-course-manager-next/blob/main/ccm_web/server/src/api/dtos/api.section.ids.dto.ts

Let me check on it one more time and get back to you.

Ok I checked again, even though this is list sectionIds.map but that list will always have 1 section for unmerge section. So the wrapper for p-limit strictly it is not needed since we are queuing one call. But I will leave this up to you

I'm gonna leave it. My thinking is that the backend logic should reflect what we support in the API, since either the frontend could change, or we may want to use the API directly in some strange customer support scenario. In all other cases where we could have >20 calls, we're using this, so it makes sense to be consistent.

ssciolla · 2022-02-09T21:30:19Z

ccm_web/server/src/canvas/canvas.service.ts

+    limit: 3,
    methods: ['POST', 'GET', 'PUT', 'DELETE'],
    statusCodes: got.defaults.options.retry.statusCodes.concat([403]),
    calculateDelay: ({ attemptCount, retryOptions, error, computedValue }) => {
-      const delay = computedValue === 0 ? 0 : 5000
+      const delay = computedValue === 0 ? 0 : 2000


I just did this. My thinking is that we initially set these so high as a way to back off from the rate limit, and get all the requests through, but that shouldn't be necessary anymore. We still want retrying behavior for intermittent Canvas errors and such, but lower levels like this should be sufficient.

ssciolla · 2022-02-09T22:01:50Z

@pushyamig and @lsloan, I think this is ready for review. Please let me know if any of the cases where I introduced limiting you think it should be removed (fine doing that, just want your input; I weighed in on the cases where it seemed less useful).

ccm_web/client/src/components/SingleSectionEnrollmentWorkflow.tsx

pushyamig · 2022-02-10T21:39:18Z

The change seems to be straight forward but affects most features. I did some preliminary testing and review, things looks good! I will go one more round tomorrow and will approve.

ccm_web/server/src/api/api.admin.handler.ts

pushyamig · 2022-02-11T21:55:40Z

Over all this is a positive change and huge performance improvement and increase customer satisfaction rate. This also means that for Add Non-UM user we could support more that 200 (based on testing).

I have my testing report across features

This is a go from me, I think just need to resolve some merge conflicts and probably remove the concurrency call with un merge sections

… use limitPromises, return functions

…ncurrentPromises; remove restraint on generic T

pushyamig

Nice Work @ssciolla

ssciolla · 2022-02-14T16:13:38Z

@pushyamig, thanks for the thorough review!

ssciolla added 🐛 bug Something isn't working back end Involves changes to the Express application or other server-related files API labels Feb 3, 2022

ssciolla requested review from lsloan and pushyamig February 3, 2022 19:18

ssciolla commented Feb 3, 2022

View reviewed changes

ccm_web/package.json Show resolved Hide resolved

ssciolla changed the title ~~Introduce limitPromises wrapper for p-limit (#326)~~ Introduce createLimitedPromises wrapper for p-limit (#326) Feb 3, 2022

ssciolla mentioned this pull request Feb 4, 2022

Observe and plan for ecosystem move to ECMAScript Modules #330

Closed

ssciolla force-pushed the issue-326-promise-limit branch from 6b9d0f1 to 1389fc7 Compare February 4, 2022 23:32

ssciolla commented Feb 5, 2022

View reviewed changes

ccm_web/server/src/api/api.utils.ts Show resolved Hide resolved

ssciolla commented Feb 9, 2022

View reviewed changes

ccm_web/server/src/api/api.course.handler.ts Show resolved Hide resolved

ssciolla commented Feb 9, 2022

View reviewed changes

ccm_web/server/src/api/api.course.handler.ts Show resolved Hide resolved

ssciolla commented Feb 9, 2022

View reviewed changes

ssciolla marked this pull request as ready for review February 9, 2022 22:00

pushyamig reviewed Feb 10, 2022

View reviewed changes

ccm_web/client/src/components/SingleSectionEnrollmentWorkflow.tsx Show resolved Hide resolved

pushyamig reviewed Feb 11, 2022

View reviewed changes

ccm_web/server/src/api/api.admin.handler.ts Show resolved Hide resolved

ssciolla added 5 commits February 11, 2022 17:09

Add p-limit; create limitPromises util; modify all promise mapping to…

31e32b1

… use limitPromises, return functions

limitPromises => createLimitedPromises; maxConcurrentRequest => maxCo…

3fa8416

…ncurrentPromises; remove restraint on generic T

Up default for maxConcurrentPromises

84626a0

Back off on number of retries and delay

5c4d23a

Modify loading messages for enrollment processes

762e466

ssciolla force-pushed the issue-326-promise-limit branch from f230f3b to 762e466 Compare February 11, 2022 22:10

pushyamig approved these changes Feb 14, 2022

View reviewed changes

Remove limiting on course search for each account

98281ca

ssciolla merged commit 3b9ca83 into tl-its-umich-edu:main Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

ssciolla commented Feb 3, 2022 •

edited

pushyamig commented Feb 4, 2022

pushyamig commented Feb 4, 2022 •

edited

pushyamig commented Feb 9, 2022

ssciolla Feb 9, 2022 •

edited

pushyamig Feb 10, 2022

ssciolla Feb 11, 2022

pushyamig Feb 11, 2022

ssciolla Feb 14, 2022

pushyamig Feb 14, 2022

ssciolla Feb 9, 2022

pushyamig Feb 11, 2022

ssciolla Feb 11, 2022

ssciolla Feb 11, 2022

pushyamig Feb 11, 2022

ssciolla Feb 11, 2022

pushyamig Feb 14, 2022

pushyamig Feb 14, 2022

ssciolla Feb 14, 2022

ssciolla Feb 9, 2022 •

edited

ssciolla commented Feb 9, 2022

pushyamig commented Feb 10, 2022

pushyamig commented Feb 11, 2022

pushyamig left a comment

ssciolla commented Feb 14, 2022

Introduce createLimitedPromises wrapper for p-limit (#326) #327

Introduce createLimitedPromises wrapper for p-limit (#326) #327

Conversation

ssciolla commented Feb 3, 2022 • edited

pushyamig commented Feb 4, 2022

pushyamig commented Feb 4, 2022 • edited

pushyamig commented Feb 9, 2022

ssciolla Feb 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssciolla Feb 9, 2022 • edited

Choose a reason for hiding this comment

ssciolla commented Feb 9, 2022

pushyamig commented Feb 10, 2022

pushyamig commented Feb 11, 2022

pushyamig left a comment

Choose a reason for hiding this comment

ssciolla commented Feb 14, 2022

Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

Introduce `createLimitedPromises` wrapper for `p-limit` (#326) #327

ssciolla commented Feb 3, 2022 •

edited

pushyamig commented Feb 4, 2022 •

edited

ssciolla Feb 9, 2022 •

edited

ssciolla Feb 9, 2022 •

edited