feat(infra): pause workers in bootstrap until queues are ready #3396

p-fernandez · 2023-05-11T16:20:12Z

What change does this PR introduce?

Pauses the workers till the queues are ready, checked by the health checks, in order to be able to start to process jobs.

Why was this change needed?

Is part of the improvements for the startup and the shutdown of the apps.

Other information (Screenshots)

Manually tested. Might require a better implementation.

linear · 2023-05-11T16:20:15Z

NV-2210 Bootstrap of app enable worker when healthchecks are passing

p-fernandez · 2023-05-11T16:22:06Z

apps/worker/src/app/health/health.module.ts

-  providers: [
-    DalServiceHealthIndicator,
-    QueueService,
-    QueueServiceHealthIndicator,
-    TriggerQueueService,
-    TriggerQueueServiceHealthIndicator,


Moved to SharedModule as I needed them there also. NestJS recommends to import it as provider in just one module.

p-fernandez · 2023-05-11T16:23:16Z

apps/worker/src/app/workflow/services/trigger-worker.service.ts

 @Injectable()
-export class TriggerProcessorQueueService extends TriggerQueueService {
+export class TriggerWorkerService extends TriggerQueueService implements INovuWorker {


Renaming as here we are instantiating a worker from BullMQ so for me the naming was confusing. Opinionated and debatable. Can roll back if needed.

The old name was like that because at some point we had two queues for the trigger: producer and processor.
I feel like the name should be similar to what we have for the other queues WorkflowQueueService, WsQueueService, so like TriggerQueueService, but I'm not sure why we have TriggerQueueService and not are reusing the QueueService

I can give it a look regarding not using QueueService. But I still think using Queue when we are instantiating a worker is confusing. Maybe here I am thinking more in AWS terms of architecture (SQS, SNS). Also we can raise that as an open question in the team. What do you think?

I wonder if separating the responsibility of the queue service to Publisher/Subscriber terms could help us in the future and will be more convenient terminology. ( most definitely not related to this pr, i would love to raise a POC PR if you think it will help)

You worded nicely @djabarovgeorge. Indeed it is the terminology that I was aiming with these changes as latest years have worked a lot with Pub/Sub tools and patterns and I personally enjoy that separation of concerns. I also see we have that separation in our implementation (it can be seen in the services created and point of discussion) so to me made sense heading that direction with the naming. But again, something worth to bring to the team.

I have been pairing with @LetItRock and we agreed to tackle the proposed refactor in a different PR, as the separation of concerns is a good thing and we can have it less confusing as right now.

p-fernandez · 2023-05-11T16:23:22Z

apps/worker/src/app/workflow/services/workflow-worker.service.ts

 @Injectable()
-export class WorkflowQueueService extends QueueService<IJobData> {
+export class WorkflowWorkerService extends QueueService<IJobData> implements INovuWorker {


Renaming as here we are instantiating a worker from BullMQ so for me the naming was confusing. Opinionated and debatable. Can roll back if needed.

Renaming and refactor will be done in a different PR.

p-fernandez · 2023-05-11T16:24:17Z

apps/worker/src/app/workflow/workflow.module.ts

-  {
-    provide: QueueService,
-    useClass: WorkflowQueueService,
-  },
-  {
-    provide: TriggerQueueService,
-    useClass: TriggerProcessorQueueService,
-  },


I think this didn't make sense. Might be very wrong.
CC: @davidsoderberg ?

@p-fernandez these are injected to the BULLMQ_LIST in line 120... also please check all the places where QueueService and TriggerQueueService were used before and change there accordingly... from what I remember is that AddJob use-case is using private queueService: QueueService in the constructor because the WorkflowQueueService is not the part of the application-generic package

p-fernandez · 2023-05-11T16:25:34Z

apps/worker/src/bootstrap.ts

@@ -71,6 +95,8 @@ export async function bootstrap(): Promise<INestApplication> {

  await app.listen(process.env.PORT);

+  await startAppInfra(app);


Doubting if we should enable the worker before or after letting the application to listen to requests.

I think that this order makes sense here

p-fernandez · 2023-05-11T16:28:16Z

packages/application-generic/src/services/readiness.service.ts

+    try {
+      const healths = await Promise.all([
+        this.queueServiceHealthIndicator.isHealthy(),
+        this.triggerQueueServiceHealthIndicator.isHealthy(),
+        this.wsQueueServiceHealthIndicator.isHealthy(),
+      ]);
+
+      return healths.every((health) => !!health === true);


In a µservices world I would do HTTP/gRPC calls to the health check endpoints of the different services we are dependant. In this case we share the health indicators in our app generic package. Underneath all them use the same BullMQ instance with its dependency to a dedicated in-memory database. So we check the 3 even though with one would be enough. This would let it prepared if we create a dedicate in-memory instance for every queue in the future.

p-fernandez · 2023-05-12T09:59:50Z

apps/worker/src/app/shared/shared.module.ts

  StorageHelperService,
+  readinessService,


Order matters. If a provider is dependant of other providers, needs to be added later in the array. Learnt it the hard way. 🙁

scopsy

Looks good! Would love to get @davidsoderberg opinion regarding the open question of the WorkflowService imports just in case, but otherwise looks good.

scopsy · 2023-05-14T17:12:44Z

apps/worker/src/bootstrap.ts

@@ -71,6 +95,8 @@ export async function bootstrap(): Promise<INestApplication> {

  await app.listen(process.env.PORT);

+  await startAppInfra(app);


I think that this order makes sense here

scopsy · 2023-05-14T17:14:05Z

packages/application-generic/src/services/readiness.service.ts

+  async pauseWorkers(workers: INovuWorker[]): Promise<void> {
+    for (const worker of workers) {
+      Logger.log(`Pausing worker ${worker.name}...`, LOG_CONTEXT);
+      await worker.pauseWorker();


I assume that pausing might take time while finishing processing inflights calls, maybe we should do Promise.all here?

Not sure if you are thinking this is for the graceful shutdown. This is used here: https://github.com/novuhq/novu/pull/3396/files#r1193495088

So it is meant to be used before we even accept calls (before app is ready to listen).

Oh okey, that makes sense. A small suggestion maybe we can utilize the autorun https://api.docs.bullmq.io/interfaces/WorkerOptions.html#autorun worker option instead of "pausing" it in the appp bootstrap? So then we only need to enable it?

Had to revert this change back as I wasn't able to find the reason tests were not working fully. We can try it in the future.

p-fernandez · 2023-05-15T08:19:04Z

apps/worker/src/bootstrap.ts

@@ -40,6 +62,8 @@ export async function bootstrap(): Promise<INestApplication> {
  app.useLogger(app.get(PinoLogger));
  app.flushLogs();

+  await prepareAppInfra(app);


We pause the workers here.

LetItRock · 2023-06-13T16:27:37Z

apps/worker/src/app/workflow/workflow.module.ts

+  QueueService,
+  TriggerQueueService,


It looks wrong to me (but I'm not sure) that we do provide: QueueService, TriggerQueueService and then also WorkflowWorkerService, TriggerWorkerService. This way we will have two instances of the same queues because the WorkflowWorkerService extends QueueService and TriggerWorkerService extends TriggerQueueService. The WorkflowWorkerService, TriggerWorkerService instances were injected by their base class because some use cases leave in the application-generic package but these classes are defined in the worker app.
I feel it's better to have what was there before.

For future readers, I have been pairing with @LetItRock and we agreed to tackle the proposed refactor in a different PR, as the separation of concerns is a good thing and we can have it less confusing as right now. Same for these dependencies and renaming. We will clarify it in that other PR.

properly in tests

p-fernandez requested review from davidsoderberg, LetItRock, ainouzgali, scopsy, BiswaViraj, djabarovgeorge and Cliftonz May 11, 2023 16:20

p-fernandez self-assigned this May 11, 2023

github-actions bot added @novu/api @novu/worker labels May 11, 2023

p-fernandez commented May 11, 2023

View reviewed changes

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch 2 times, most recently from 0b5a557 to bfb47ac Compare May 12, 2023 09:55

p-fernandez commented May 12, 2023

View reviewed changes

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch from bfb47ac to 2161f31 Compare May 12, 2023 10:09

Base automatically changed from nv-2137-graceful-shutdown-and-bootstrap-for to next May 12, 2023 16:20

scopsy approved these changes May 14, 2023

View reviewed changes

p-fernandez commented May 15, 2023

View reviewed changes

p-fernandez requested a review from scopsy May 17, 2023 16:04

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch from 1345e22 to ba80c13 Compare May 30, 2023 16:23

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch 2 times, most recently from a0ad2fc to 534ca5d Compare June 13, 2023 15:36

LetItRock reviewed Jun 13, 2023

View reviewed changes

feat(infra): pause workers in bootstrap until queues are ready

2c0cc3f

p-fernandez added 2 commits June 15, 2023 16:12

fix(infra): pr comments

4e2d61e

fix: reviewing dependencies and extension

61eac1c

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch from 534ca5d to 61eac1c Compare June 15, 2023 15:13

p-fernandez added 2 commits June 15, 2023 16:37

fix: having to revert back the autorun flag as it is not working

a936b2b

properly in tests

feat(worker): leave existing names to address changes in separate pr

6f67938

p-fernandez force-pushed the nv-2210-bootstrap-of-app-enable-worker-when branch from 8dd9e20 to 6f67938 Compare June 16, 2023 11:29

p-fernandez added this pull request to the merge queue Jun 16, 2023

Merged via the queue into next with commit c78b8d1 Jun 16, 2023

p-fernandez deleted the nv-2210-bootstrap-of-app-enable-worker-when branch June 16, 2023 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(infra): pause workers in bootstrap until queues are ready #3396

feat(infra): pause workers in bootstrap until queues are ready #3396

p-fernandez commented May 11, 2023

linear bot commented May 11, 2023

p-fernandez May 11, 2023

p-fernandez May 11, 2023

LetItRock Jun 12, 2023

p-fernandez Jun 13, 2023

djabarovgeorge Jun 13, 2023

p-fernandez Jun 13, 2023

p-fernandez Jun 16, 2023

p-fernandez May 11, 2023

p-fernandez Jun 16, 2023

p-fernandez May 11, 2023

LetItRock Jun 12, 2023

p-fernandez May 11, 2023

scopsy May 14, 2023

p-fernandez May 11, 2023

p-fernandez May 12, 2023

scopsy left a comment

scopsy May 14, 2023

scopsy May 14, 2023

p-fernandez May 15, 2023

scopsy May 15, 2023

p-fernandez Jun 15, 2023

p-fernandez May 15, 2023

LetItRock Jun 13, 2023

p-fernandez Jun 16, 2023

		@@ -71,6 +95,8 @@ export async function bootstrap(): Promise<INestApplication> {

		await app.listen(process.env.PORT);

		await startAppInfra(app);

feat(infra): pause workers in bootstrap until queues are ready #3396

feat(infra): pause workers in bootstrap until queues are ready #3396

Conversation

p-fernandez commented May 11, 2023

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopsy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment