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

#3667 - Bull Scheduler Increments the Next Scheduled Job When Manually Promoted #3957

Merged
merged 6 commits into from
Nov 19, 2024

Conversation

andrewsignori-aot
Copy link
Collaborator

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

Updated Bull Board-related packages to take advantage of the feature present in the newer version that allows a new job to be added (same as duplicate), allowing the schedulers to be executed without affecting their current time to be executed again.

Notes:

  1. during the local tests, when a delayed job was deleted and the queue-consumers restarted, the delayed job was restored.
  2. this PR is intended to be part of the upcoming release. The research for the ticket will continue.

Using the add/duplicate option

While creating the new job the properties can be edited, for instance, the corn expression can be edited to create a new time to execute the scheduler.

Please note that timestamp and prevMillis should be removed. These properties are generated once the job is created.
As mentioned in the source code, timestamp is the "Timestamp when the job was created." and prevMillis is a "Internal property used by repeatable jobs."

image

{
  "repeat": {
    "count": 1,
    "key": "__default__::::0 7 * * *",
    "cron": "0 7 * * *"
  },
  "jobId": "repeat:2c2720c5e8b4e9ce99993becec27a0ff:1731999600000",
  "delay": 42485905,
  "timestamp": 1731957114095,
  "prevMillis": 1731999600000,
  "attempts": 3,
  "backoff": {
    "type": "fixed",
    "delay": 180000
  }
}

Refactor

  • Refactored the code to move the Bull Board configuration to the Nestjs modules instead of doing it on the main.ts.
    The refactored code is equivalent to the one previously on the main.ts.
  • Updated icons and labels to make the dashboard look more like part of the SIMS. The way it was done was by targeting less effort. In case it causes some noise during the PR review the code will be removed.

@andrewsignori-aot andrewsignori-aot self-assigned this Nov 18, 2024
@andrewsignori-aot andrewsignori-aot marked this pull request as ready for review November 18, 2024 21:02
@dheepak-aot dheepak-aot self-requested a review November 18, 2024 21:32
@@ -25,6 +30,12 @@ import { DatabaseModule } from "@sims/sims-db";
inject: [ConfigService],
}),
BullModule.registerQueueAsync(...getQueueModules()),
BullBoardModule.forRootAsync({
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed, the queue module is shared with API (to publish messages to queue) and loading bull board module here will stand up the bull dashboard in SIMS API as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The modules are now added only to the queue-consumers.

* @returns Bull Board module options with the dashboard route,
* authentication middleware and the board options.
*/
async function bullBoardModuleFactory(configService: ConfigService) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please specify the return type in the method.

async function bullBoardModuleFactory(
  configService: ConfigService,
): Promise<BullBoardModuleOptions>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Return type added.

/**
* Adds all queues to the bull board during application initialization.
* checking if the queue is active and if it is a scheduler.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor please check the comment and period.

Also please add returns.

Copy link
Collaborator Author

@andrewsignori-aot andrewsignori-aot Nov 19, 2024

Choose a reason for hiding this comment

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

Add the period.
About the return type, I do not believe that we usually add comments for Promise void. Please let me know if that is what you meant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are right about the return type.

Comment on lines 127 to 132
boardLogo: {
path: "https://sims.studentaidbc.ca/favicon-32x32.png",
},
favIcon: {
default: "https://sims.studentaidbc.ca/favicon-16x16.png",
alternative: "https://sims.studentaidbc.ca/favicon-32x32.png",
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️Looks great with SIMS logo.

@dheepak-aot
Copy link
Collaborator

Great Job! Please have a look at the comments. Except one rest all are minor.

Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @andrewsignori-aot

Comment on lines +51 to +52
replace: BullBoardQueuesModule,
by: BullBoardQueuesModuleMock,
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.

Thanks for doing the changes. Looks good 👍

Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 22.1% ( 3718 / 16821 )
Methods: 10.19% ( 214 / 2100 )
Lines: 25.44% ( 3226 / 12680 )
Branches: 13.62% ( 278 / 2041 )

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: 86.93% ( 1244 / 1431 )
Methods: 88.57% ( 124 / 140 )
Lines: 87.78% ( 1056 / 1203 )
Branches: 72.73% ( 64 / 88 )

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 )

@andrewsignori-aot andrewsignori-aot added this pull request to the merge queue Nov 19, 2024
Merged via the queue into main with commit 7b8df04 Nov 19, 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