-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Refactor the broadcast method to utilize the new queue #4526
Changes from 6 commits
c599eee
41dc6bd
092193e
908f7dc
92bb3ae
d1204c0
3931058
8a80504
010ff5e
b0d91a1
601b977
61449fd
e44c267
0079dc1
775a87b
bc81155
3bd3a71
81f5f0d
02614f8
d0d22e8
1de3c4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was debating with myself if to update the small note, if |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
import { TriggerEventStatusEnum } from '@novu/shared'; | ||
import { MapTriggerRecipients } from '@novu/application-generic'; | ||
|
||
import { ProcessBulkTriggerCommand } from './process-bulk-trigger.command'; | ||
|
||
|
@@ -10,7 +9,7 @@ import { ParseEventRequest } from '../parse-event-request/parse-event-request.us | |
|
||
@Injectable() | ||
export class ProcessBulkTrigger { | ||
constructor(private parseEventRequest: ParseEventRequest, private mapTriggerRecipients: MapTriggerRecipients) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redundant 🧹 |
||
constructor(private parseEventRequest: ParseEventRequest) {} | ||
|
||
async execute(command: ProcessBulkTriggerCommand) { | ||
const results: TriggerEventResponseDto[] = []; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,62 +1,34 @@ | ||
import { Injectable } from '@nestjs/common'; | ||
import * as _ from 'lodash'; | ||
import { SubscriberEntity, SubscriberRepository } from '@novu/dal'; | ||
import { TriggerEvent, TriggerEventCommand } from '@novu/application-generic'; | ||
import { TriggerEventStatusEnum } from '@novu/shared'; | ||
import { SubscriberRepository } from '@novu/dal'; | ||
import { AddressingTypeEnum, TriggerEventStatusEnum } from '@novu/shared'; | ||
|
||
import { TriggerEventToAllCommand } from './trigger-event-to-all.command'; | ||
import { ParseEventRequest, ParseEventRequestCommand } from '../parse-event-request'; | ||
|
||
@Injectable() | ||
export class TriggerEventToAll { | ||
constructor(private triggerEvent: TriggerEvent, private subscriberRepository: SubscriberRepository) {} | ||
constructor(private subscriberRepository: SubscriberRepository, private parseEventRequest: ParseEventRequest) {} | ||
|
||
public async execute(command: TriggerEventToAllCommand) { | ||
const batchSize = 500; | ||
let list: SubscriberEntity[] = []; | ||
|
||
for await (const subscriber of this.subscriberRepository.findBatch( | ||
{ | ||
_environmentId: command.environmentId, | ||
_organizationId: command.organizationId, | ||
}, | ||
'subscriberId', | ||
{}, | ||
batchSize | ||
)) { | ||
list.push(subscriber); | ||
if (list.length === batchSize) { | ||
await this.trigger(command, list); | ||
list = []; | ||
} | ||
} | ||
|
||
if (list.length > 0) { | ||
await this.trigger(command, list); | ||
} | ||
|
||
return { | ||
acknowledged: true, | ||
status: TriggerEventStatusEnum.PROCESSED, | ||
transactionId: command.transactionId, | ||
}; | ||
} | ||
|
||
private async trigger(command: TriggerEventToAllCommand, list: SubscriberEntity[]) { | ||
await this.triggerEvent.execute( | ||
TriggerEventCommand.create({ | ||
await this.parseEventRequest.execute( | ||
ParseEventRequestCommand.create({ | ||
userId: command.userId, | ||
environmentId: command.environmentId, | ||
organizationId: command.organizationId, | ||
identifier: command.identifier, | ||
payload: command.payload, | ||
to: list.map((item) => ({ | ||
subscriberId: item.subscriberId, | ||
})), | ||
payload: command.payload || {}, | ||
addressingType: AddressingTypeEnum.BROADCAST, | ||
transactionId: command.transactionId, | ||
overrides: command.overrides, | ||
overrides: command.overrides || {}, | ||
actor: command.actor, | ||
tenant: command.tenant, | ||
}) | ||
); | ||
|
||
return { | ||
acknowledged: true, | ||
status: TriggerEventStatusEnum.PROCESSED, | ||
transactionId: command.transactionId, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,6 @@ export class HealthController { | |
private healthCheckService: HealthCheckService, | ||
private cacheHealthIndicator: CacheServiceHealthIndicator, | ||
private dalHealthIndicator: DalServiceHealthIndicator, | ||
private standardQueueHealthIndicator: StandardQueueServiceHealthIndicator, | ||
private workflowQueueHealthIndicator: WorkflowQueueServiceHealthIndicator | ||
) {} | ||
|
||
|
@@ -26,7 +25,6 @@ export class HealthController { | |
healthCheck(): Promise<HealthCheckResult> { | ||
const checks: HealthIndicatorFunction[] = [ | ||
async () => this.dalHealthIndicator.isHealthy(), | ||
async () => this.standardQueueHealthIndicator.isHealthy(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We no longer need it because we will use only the workflow queue in the API. |
||
async () => this.workflowQueueHealthIndicator.isHealthy(), | ||
async () => { | ||
return { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,21 @@ | ||
import { MiddlewareConsumer, Module, NestModule } from '@nestjs/common'; | ||
import { CompileTemplate, QueuesModule } from '@novu/application-generic'; | ||
import { BaseApiQueuesModule, CompileTemplate } from '@novu/application-generic'; | ||
|
||
import { USE_CASES } from './usecases'; | ||
import { InboundParseController } from './inbound-parse.controller'; | ||
import { InboundParseQueueService } from './services/inbound-parse.queue.service'; | ||
import { GetMxRecord } from './usecases/get-mx-record/get-mx-record.usecase'; | ||
import { InboundEmailParse } from './usecases/inbound-email-parse/inbound-email-parse.usecase'; | ||
|
||
import { SharedModule } from '../shared/shared.module'; | ||
import { AuthModule } from '../auth/auth.module'; | ||
|
||
const PROVIDERS = [InboundParseQueueService, GetMxRecord, CompileTemplate]; | ||
|
||
@Module({ | ||
imports: [SharedModule, AuthModule, QueuesModule], | ||
imports: [SharedModule, AuthModule, BaseApiQueuesModule], | ||
controllers: [InboundParseController], | ||
providers: [...PROVIDERS, ...USE_CASES], | ||
exports: [...USE_CASES, QueuesModule], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to export this one here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It probably not anymore. I think it can be safe to be removed. |
||
exports: [...USE_CASES], | ||
}) | ||
export class InboundParseModule implements NestModule { | ||
configure(consumer: MiddlewareConsumer): MiddlewareConsumer | void {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,9 +100,8 @@ const PROVIDERS = [ | |
version: packageJson.version, | ||
}) | ||
), | ||
QueuesModule, | ||
], | ||
providers: [...PROVIDERS], | ||
exports: [...PROVIDERS, LoggerModule, QueuesModule], | ||
Comment on lines
-101
to
-104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why we need this one here therefore I removed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was added to provide the QueuesModule as provider in any module that is being dependant of SharedModule so in case of need QueuesModule would be available without having to inject it in said module. |
||
exports: [...PROVIDERS, LoggerModule], | ||
}) | ||
export class SharedModule {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
export { QueuesModule } from './queues.module'; | ||
export { BaseApiQueuesModule } from './queues.module'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can reuse the same export line. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,3 +67,19 @@ const PROVIDERS: Provider[] = [ | |
exports: [...PROVIDERS], | ||
}) | ||
export class QueuesModule {} | ||
|
||
const APP_PROVIDERS: Provider[] = [ | ||
InboundParseQueue, | ||
InboundParseWorker, | ||
InboundParseQueueServiceHealthIndicator, | ||
WebSocketsQueueService, | ||
WebSocketsQueueServiceHealthIndicator, | ||
WorkflowQueueService, | ||
WorkflowQueueServiceHealthIndicator, | ||
]; | ||
|
||
@Module({ | ||
providers: [...APP_PROVIDERS], | ||
exports: [...APP_PROVIDERS], | ||
}) | ||
export class BaseApiQueuesModule {} | ||
Comment on lines
+70
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you trying to gather all the QueuesModule dependencies used only in API app? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes exactly, for two reasons
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, now explained it is a great idea and didn't think about it (even though the clients should be only initialised in the custom providers if needed) and the services only when injected, if I am not wrong. I am happy to take that responsibility in a following task to this one so you don't have to take that work as we need this to move forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the small test I made locally before making the changes I saw that they do initialize even if not injected, i guess because they are singletons maybe? Regarding the separation i totally agree, make it only for the API in order to keep the scope as small as possible for this PR as it is an extra for it. We definitely can update the other ones as well. Once again I agree we can merge this one for now, the additional improvements can wait for now. I made this one as a preparation for the future. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,13 @@ import { | |
IsString, | ||
IsOptional, | ||
ValidateNested, | ||
IsEnum, | ||
} from 'class-validator'; | ||
import { ISubscribersDefine, ITenantDefine } from '@novu/shared'; | ||
import { | ||
AddressingTypeEnum, | ||
ISubscribersDefine, | ||
ITenantDefine, | ||
} from '@novu/shared'; | ||
|
||
import { EnvironmentWithUserCommand } from '../../commands'; | ||
|
||
|
@@ -19,8 +24,8 @@ export class TriggerEventCommand extends EnvironmentWithUserCommand { | |
@IsDefined() | ||
overrides: Record<string, Record<string, unknown>>; | ||
|
||
@IsDefined() | ||
to: ISubscribersDefine[]; | ||
@IsOptional() | ||
to?: ISubscribersDefine[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason behind making this optional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question, it is related to this thought i had There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, wasn't sure what that meant. |
||
|
||
@IsString() | ||
@IsDefined() | ||
|
@@ -33,4 +38,7 @@ export class TriggerEventCommand extends EnvironmentWithUserCommand { | |
@IsOptional() | ||
@ValidateNested() | ||
tenant?: ITenantDefine | null; | ||
|
||
@IsOptional() | ||
addressingType?: AddressingTypeEnum; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i suspect we have more redundant providers in the DI.