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

#3666 - Queue Monitoring - Refactoring Schedulers #4032

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

andrewsignori-aot
Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot commented Dec 5, 2024

  • Extended the BaseScheduler from the BaseQueue to enforce all the same basic features like porcessSummary and error handling, as agreed during PR #3666 - Queue Monitoring - Default Queue Error Handling #4020.

  • Started the refactor os some scheduler to be used as a baseline for the upcoming refactors.

    • cas-supplier-integration.scheduler.ts
    • cra-process-integration.scheduler.ts
    • cra-response-integration.scheduler.ts
  • Added the below code to every scheduler to be refactored.
    Note: this is also responsible for the Sonarcloud duplication issue. There are 23 files that have exactly the same change.

/**
 * To be removed once the method {@link process} is implemented.
 * This method "hides" the {@link Process} decorator from the base class.
 */
async processQueue(): Promise<string | string[]> {
  throw new Error("Method not implemented.");
}

/**
 * When implemented in a derived class, process the queue job.
 * To be implemented.
 */
protected async process(): Promise<string | string[]> {
  throw new Error("Method not implemented.");
}

@andrewsignori-aot andrewsignori-aot marked this pull request as ready for review December 11, 2024 17:03
@dheepak-aot dheepak-aot self-requested a review December 11, 2024 17:28
@@ -49,7 +49,7 @@ export class CancelApplicationAssessmentProcessor extends BaseQueue<CancelAssess
* @param job information to perform the process.
* @param processSummary process summary for logging.
* @returns processing result. */
async process(
protected async process(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

appLogger: this.logger,
jobLogger: job,
});
await summary.info("Processing pending applications to be archived.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor. the info message can be retained.

import { QueueService } from "@sims/services/queue";
import { QueueNames } from "@sims/utilities";
import { ConfigService } from "@sims/utilities/config";
import { InjectLogger } from "@sims/utilities/logger";
import { InjectLogger, LoggerService } from "@sims/utilities/logger";
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

* To be removed once the method {@link process} is implemented.
* This method "hides" the {@link Process} decorator from the base class.
*/
async processQueue(): Promise<string | string[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have the default implementation in the base class itself so that we don't have to repeat in other places? - Is that approach not working?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found that when class(indluding subclass) decorated twice the bull doesn't accept it.
image

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 Job! Just left one minor comment.

@andrepestana-aot andrepestana-aot self-requested a review December 11, 2024 22:08
protected async process(
_job: Job<void>,
processSummary: ProcessSummary,
): Promise<string | string[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return can be Promise<string[]>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed!

this.logger.log(
`Processing CRA integration job ${job.id} of type ${job.name}.`,
);
protected async process(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add processSummary to comments.

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!

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.

Great job with the refactoring. Just minor comments.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
6.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 21.82% ( 3765 / 17257 )
Methods: 9.81% ( 215 / 2191 )
Lines: 25.21% ( 3270 / 12972 )
Branches: 13.37% ( 280 / 2094 )

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 Queue Consumers Coverage Report

Totals Coverage
Statements: 80% ( 1336 / 1670 )
Methods: 63.55% ( 136 / 214 )
Lines: 83.49% ( 1123 / 1345 )
Branches: 69.37% ( 77 / 111 )

Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 67.41% ( 5927 / 8792 )
Methods: 65.09% ( 729 / 1120 )
Lines: 71.34% ( 4650 / 6518 )
Branches: 47.49% ( 548 / 1154 )

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