-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
add message queue integration #2491
Conversation
05edad7
to
8246fff
Compare
export const QUEUE_DRIVER = Symbol('QUEUE_DRIVER'); | ||
|
||
export enum QUEUE_TOPICS { | ||
taskAssignedQueueTopic = 'task-assigned-topic', |
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.
1 queue = 1 broader category, no? Like "notifications" would be a single queue in my opinion, not one queue topic per notification. The main benefit I see of splitting into multiple queues would be to prioritize/protect one queue from blocking another (e.g. email overload doesn't block payment or whatever more important queue) and for debugging
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.
correct each queue will handle specific event, i was not looking to have a single queue for all notifications, because some notifications could be more important and some notifcations are triggered very often, so its better to separate queue based on actions or use cases, this way worker can do "handle" the event, for example, we might want to send email when user signs up also might want to create some external accounts, all these could be handled by signup
event and worker.
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.
also if we have separate queue for notifications, producers would have to publish to notification queue separately, apart from the async tasks queue they might be publishing, so it makes publishers to be coupled with the notifications, so I feel we should avoid that.
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.
Ah yes I get what you mean, something more event-based than action-based.
But even if we do event-based I'm not sure if this isn't too much granularity? Having one queue topic per events feels off to me. But I don't have much experience with queue, I'll let someone else give his opinion!
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 agree with @mahendraHegde on this, but I think the decision is more on product side, because it really depend on our needs.
If we answer yes to this question, multiple queues is the way to go:
Do we need to prioritize certain types of events over others ?
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 feel like the reason it's confusing is because we are mixing two separate things:
(1) building a pub/sub system with events emitters and listeners
(2) building a mechanism for asynchronous job processing
Both are covered by NestJS with core modules, but the core module for (2) adds a dependence on Redis which we don't want. We should stick to rebuild (2), I feel like here we are trying to rebuild both
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.
@FelixMalfait you are right, My bad, im going to change naming convention to avoid any confusions, intention is not to build the pub/sub but to build a queue for async job processing.
however that makes local driver implementation difficult, nodejs event emiter is pub/sub by default(multiple consumers will get same event), so implementing a local sync queue would require some custom implementation(basically in memory queue), so my question is,is local driver even needed? as mentioned before pg-boss
doesnt require any additional setup, so it makes sense to use it as primary driver for local development also wdyt?
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.
Yes I agree, I'm not sure the local driver is relevant either since, as you say, Postgres is already there.
So maybe there's a little over-engineering in the current solution? It's sometimes frustrating to remove code but we wouldn't have discovered/understood this without your code
Also @magrinj found this package:
https://www.npmjs.com/package/@nestjs-enhanced/pg-boss that is another wrapper. But the code isn't even on Github. It's not something we should use - just referencing for additional inspiration
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.
@FelixMalfait I have pushed changes, also added example usage in doc, let me know if you still want me to follow the decorator way of handling stuff as done by the above npm library. i feel this approach is simpler.
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.
as per the worker, we have 2 options
- run the worker part of the server process.
- run it as a standalone nest process
i'm planing to go with 2nd approach, so that we get the max of the queue aproach and dont block the main proces's event loop when we do some heavy computation in the future,
server/src/integrations/message-queue/drivers/driver.constants.ts
Outdated
Show resolved
Hide resolved
server/src/integrations/message-queue/services/message-queue-task-assigned.service.spec.ts
Outdated
Show resolved
Hide resolved
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.
Great work! Thanks
8246fff
to
a0a1028
Compare
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.
Really good job, thanks a lot for your work ! 🎉
server/src/database/typeorm/metadata/migrations/1699890270187-setupMetadataTables.ts
Outdated
Show resolved
Hide resolved
export const QUEUE_DRIVER = Symbol('QUEUE_DRIVER'); | ||
|
||
export enum QUEUE_TOPICS { | ||
taskAssignedQueueTopic = 'task-assigned-topic', |
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 agree with @mahendraHegde on this, but I think the decision is more on product side, because it really depend on our needs.
If we answer yes to this question, multiple queues is the way to go:
Do we need to prioritize certain types of events over others ?
219d836
to
5b73680
Compare
5b73680
to
4f8cc73
Compare
@magrinj @FelixMalfait i have made the decorator change as we discussed. |
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.
@mahendraHegde Great jobs ! Few changes requested :)
I saw decorators on @nestjs/bull
allowing the creation of consumer class, maybe it's something we can add later !
server/src/integrations/message-queue/message-queue.constants.ts
Outdated
Show resolved
Hide resolved
4f8cc73
to
1771f48
Compare
@mahendraHegde Perfect, I've approved ! |
1771f48
to
3fa0700
Compare
CLA
Hello there and welcome to our project! |
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.
A huge thanks! This is deeply appreciated. Merging it!
A note about the queue, I think it should not be restricted to "message" queue. It should be a global queue feature that we could use for anything
export const QUEUE_DRIVER = Symbol('QUEUE_DRIVER'); | ||
|
||
export enum MessageQueues { | ||
taskAssignedQueue = 'task-assigned-queue', |
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 think this should be less specific but we can work on that later :)
We should have a notificationQueue, paymentQueue, ...
closes #1645
closes #827