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

#3260 - CAS Integration 3A - Create new Supplier and Site - Process in Parallel #3837

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

andrewsignori-aot
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot commented Oct 24, 2024

  • Adjusted supplier's processes to happen in parallel.
  • Reduced the amount of allowed parallel processes to 2 to 4. Previously it was using the "os cores" as the default value which could potentially be up to 32 while running on Openshift.

@andrewsignori-aot andrewsignori-aot marked this pull request as ready for review October 24, 2024 22:55
@andrepestana-aot andrepestana-aot self-requested a review October 24, 2024 22:59
@andrepestana-aot
Copy link
Collaborator

andrepestana-aot commented Oct 24, 2024

  • Adjusted supplier's processes to happen in parallel.
  • Reduced the amount of allowed parallel processes to 2 to 4.

" Increased the amount of allowed parallel processes from 2 to 4."

/**
* Number of parallel processes allowed to be started at same time.
*/
export enum ParallelIntensity {
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

@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!

@@ -48,6 +54,9 @@ export class CASSupplierIntegrationService {
parentProcessSummary: ProcessSummary,
studentSuppliers: StudentSupplierToProcess[],
): Promise<number> {
// Force the CAS token to be acquired before starting the process.
await this.casService.getToken();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great. When the get token fails, process is aborted right away.

@andrewsignori-aot
Copy link
Collaborator Author

andrewsignori-aot commented Oct 24, 2024

  • Adjusted supplier's processes to happen in parallel.
  • Reduced the amount of allowed parallel processes to 2 to 4.

" Increased the amount of allowed parallel processes from 2 to 4."

My comment was not clear, I meant that the default value was decreased from the "os.cores" amount (potentially 32 on Openshift) to 2 or 4 😉
I edited the PR description to be clear.

@@ -48,6 +54,9 @@ export class CASSupplierIntegrationService {
parentProcessSummary: ProcessSummary,
studentSuppliers: StudentSupplierToProcess[],
): Promise<number> {
// Force the CAS token to be acquired before starting the process.
await this.casService.getToken();
Copy link
Collaborator

@dheepak-aot dheepak-aot Oct 24, 2024

Choose a reason for hiding this comment

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

Minor and not a blocker, the method we are looking here is like initToken right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the same method used internally by the method to get a token (request one or cached one).
There is no guarantee that this method will be the one initializing the token, for instance, if the scheduler is retrying due to some individual student error the token will still be cached.

The method getToken can be kept private (as it was before) and we can create something like ensureTokenInitialization.
I reused the same method for simplicity. Please let me know if it would be a blocker or if it requires a quick chat 😉

@dheepak-aot
Copy link
Collaborator

Thank you very much for making the change. minor comments and a question.

Copy link

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.58% ( 3667 / 16243 )
Methods: 10.19% ( 206 / 2022 )
Lines: 25.96% ( 3183 / 12260 )
Branches: 14.18% ( 278 / 1961 )

Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 58.64% ( 509 / 868 )
Methods: 52.88% ( 55 / 104 )
Lines: 62.27% ( 411 / 660 )
Branches: 41.35% ( 43 / 104 )

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.

Thanks for making the change and nice conversation. Looks good 👍

Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 83.87% ( 1175 / 1401 )
Methods: 84.29% ( 118 / 140 )
Lines: 84.78% ( 997 / 1176 )
Branches: 70.59% ( 60 / 85 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 66.01% ( 5571 / 8439 )
Methods: 63.3% ( 683 / 1079 )
Lines: 70.12% ( 4401 / 6276 )
Branches: 44.93% ( 487 / 1084 )

@andrewsignori-aot andrewsignori-aot added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit 1a11ac0 Oct 25, 2024
20 checks passed
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