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.

Sign up for GitHub

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

Jump to bottom

[#11039] Synchronize instructor create calls from frontend to prevent race condition #11063

Closed
wants to merge 10 commits into from
Closed

[#11039] Synchronize instructor create calls from frontend to prevent race condition #11063

wants to merge 10 commits into from

Conversation

moziliar
Copy link
Contributor

@moziliar moziliar commented Mar 25, 2021 •
edited

Fixes #11039

Outline of Solution

Wrap the original addInstructor in a promise returning function asyncAddInstructor, leave the original function signature unchanged, and use await in addAllInstructors to create blocking effect.

Before
image

After
image

@moziliar moziliar force-pushed the 11039-sequentialize-account-create-api-call branch from ad2cb6a to 82ee5f4 Compare March 25, 2021 08:41
@moziliar moziliar mentioned this pull request Mar 25, 2021
Closed
@t-cheepeng t-cheepeng added the s.ToReview The PR is waiting for review(s) label Mar 26, 2021
@moziliar moziliar force-pushed the 11039-sequentialize-account-create-api-call branch from cc2f5d8 to 4c8777c Compare March 28, 2021 16:45
@moziliar moziliar force-pushed the 11039-sequentialize-account-create-api-call branch from 4c8777c to bd58f31 Compare March 28, 2021 16:56
@moziliar moziliar force-pushed the 11039-sequentialize-account-create-api-call branch from 6c396c7 to 354f1c2 Compare March 29, 2021 02:06
@moziliar moziliar force-pushed the 11039-sequentialize-account-create-api-call branch from c7ba419 to 27b743a Compare March 29, 2021 03:46
@moziliar moziliar changed the title [#11039] Wrap instructor creation in promise and provide blocking addAll workflow [#11039] Synchronize instructor create calls from frontend to prevent race condition Mar 29, 2021
daongochieu2810
daongochieu2810 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@daongochieu2810 daongochieu2810 left a comment

Choose a reason for hiding this comment

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

lgtm

t-cheepeng
t-cheepeng approved these changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@t-cheepeng t-cheepeng left a comment

Choose a reason for hiding this comment

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

Other than the small comment, LGTM

src/web/app/pages-admin/admin-home-page/admin-home-page.component.ts Outdated Show resolved Hide resolved
@moziliar moziliar removed the s.ToReview The PR is waiting for review(s) label Apr 2, 2021
@moziliar moziliar added the s.FinalReview The PR is ready for final review label Apr 2, 2021
@madanalogy madanalogy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Apr 12, 2021
@madanalogy madanalogy added this to the V7.15.0 milestone Apr 12, 2021
@wkurniawan07
Copy link
Member

Sorry, but I'd like to block this PR for now. It has its benefits, but it doesn't address the real root cause.

@wkurniawan07 wkurniawan07 added the s.DoNotMerge The pull request may be ready to merge but should not be, pending some event label Apr 12, 2021
@wkurniawan07 wkurniawan07 removed this from the V7.15.0 milestone Apr 12, 2021
@madanalogy madanalogy removed the s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging label Apr 13, 2021
@wkurniawan07 wkurniawan07 removed their request for review July 10, 2021 15:18
@madanalogy
Copy link
Contributor

Issue no longer relevant, resolved with #11322

@madanalogy madanalogy closed this Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcwong hcwong hcwong approved these changes

@t-cheepeng t-cheepeng t-cheepeng approved these changes

@daongochieu2810 daongochieu2810 daongochieu2810 approved these changes

Assignees

@hcwong hcwong

@t-cheepeng t-cheepeng

@daongochieu2810 daongochieu2810

Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.DoNotMerge The pull request may be ready to merge but should not be, pending some event
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Admin create accounts: make all sample students unregistered
6 participants
@moziliar @wkurniawan07 @madanalogy @hcwong @t-cheepeng @daongochieu2810