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

#3887 - Limit Parallel Processing #3933

Merged
merged 11 commits into from
Nov 18, 2024

Conversation

andrewsignori-aot
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot commented Nov 14, 2024

  • Refactoring methods that rely on OS CPUs as a parallel limit to use the existing processInParallel method.
  • New options added to the processInParallel to keep the existing features from the methods refactored.
    • currentRecord allows progress report (added to satisfy SFAS integration and also used for Fed restrictions now).
    • partialResults allow access to lasted awaited promises batch (added to satisfy offering bulk upload).
  • While doing the review, please try to identify existing code that was just moved vs new code.

Federal Restriction Benchmark

The amount of parallel promises allowed was reduced from 4 to 2. Even though the bulk size of 750 was faster in local tests it was kept as 500 to avoid overloading the DB.

  • 2 parallel promises, 500 bulk inserted records, inserting records time: 2:24.584 (m:ss.mmm)
  • 4 parallel promises, 500 bulk inserted records, inserting records time: 2:25.388 (m:ss.mmm)
  • 2 parallel promises, 250 bulk inserted records, inserting records time: 4:24.722 (m:ss.mmm)
  • 2 parallel promises, 750 bulk inserted records, inserting records: 1:51.329 (m:ss.mmm)

Bulk Offering Upload

  • Tested importing 1000 new offerings and it was imported almost instantaneously.

@andrewsignori-aot andrewsignori-aot changed the title #3887 - Feature/#3887 limit parallel processing #3887 - Limit Parallel Processing Nov 14, 2024
@andrewsignori-aot andrewsignori-aot marked this pull request as ready for review November 14, 2024 23:39
@dheepak-aot dheepak-aot self-requested a review November 15, 2024 16:22
Comment on lines +41 to +42
progress?: (currentRecord: number) => Promise<void> | void;
partialResults?: (results: P[]) => Promise<void> | void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Amazing way to extend the the capability of the method. ❤️

Comment on lines +41 to +42
progress?: (currentRecord: number) => Promise<void> | void;
partialResults?: (results: P[]) => Promise<void> | void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about processPartialResults ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like Dheepak's suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For progress it could be updateProgress.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

processPartialResults can indicate that a process will be executed with the records, which may be or may not be the case.
Yes, the current method consuming it is executing some processing, but the processInParallel is just communicating that some partial results are done.
Between progress and updateProgress I would stick with progress.
Please let me know if some of those are not suggestions.

{
progress: (currentBulk) => {
this.logger.log(
`Inserted record bulk ${currentBulk} of ${fedRestrictionsBulks.length}.`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@dheepak-aot dheepak-aot left a comment

Choose a reason for hiding this comment

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

Great work refactoring the existing process and updating the process in parallel framework.

}
} catch (error: unknown) {
result.errorsSummary.push(
"Error while generating notifications. See logs for details",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Period at the end of the phrase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the period.

Copy link
Collaborator

@andrepestana-aot andrepestana-aot left a comment

Choose a reason for hiding this comment

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

Looks good! Great job! Just minor comments.

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.21% ( 3718 / 16740 )
Methods: 10.24% ( 214 / 2089 )
Lines: 25.57% ( 3226 / 12614 )
Branches: 13.65% ( 278 / 2037 )

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 86.92% ( 1243 / 1430 )
Methods: 88.57% ( 124 / 140 )
Lines: 87.77% ( 1055 / 1202 )
Branches: 72.73% ( 64 / 88 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 65.43% ( 583 / 891 )
Methods: 59.26% ( 64 / 108 )
Lines: 68.54% ( 464 / 677 )
Branches: 51.89% ( 55 / 106 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 66.96% ( 5773 / 8621 )
Methods: 64.8% ( 718 / 1108 )
Lines: 70.89% ( 4530 / 6390 )
Branches: 46.75% ( 525 / 1123 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants