-
Notifications
You must be signed in to change notification settings - Fork 3.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
feat(worker): add topics fanout in batches by cursor #5165
feat(worker): add topics fanout in batches by cursor #5165
Conversation
❌ Deploy Preview for dev-web-novu failed.
|
packages/application-generic/src/usecases/trigger-multicast/trigger-multicast.usecase.ts
Outdated
Show resolved
Hide resolved
packages/application-generic/src/usecases/trigger-multicast/trigger-multicast.usecase.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.
I tried to separate the test as much as possible, meaning creating some unit tests for the stateless function, and adding them in a separate file so it would be more organized but in the e2e folder so it would be close to the usecase tests.
Could not put it close to the usecase itself because we do not run at the moment test that is located inside of application-generic as far as i know.
sendToProcessSubscriberServiceStub.restore(); // Restore the original method after each test | ||
}); | ||
|
||
describe('E2E tests', () => { |
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.
these are more unit tests, not e2e, you just initialize topics with an endpoint call, but you can do it also with the usecase...
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.
This describe
is a leftover i had other before refactoring.
I looked at this as an e2e for the usecase, while we have the api full flow e2e under the /events/trigger
e2e.
do you think we should have made those e2e tests on another level?
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 mean it's unit tests because we are testing the logic of the use case, which is a single unit (class).
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.
amazing work George! 🔥
What change does this PR introduce?
Currently the process to pull all of the subscribers for a topic works until we start having the size of 2 million. This gives us a clue that we have not optimized this database call to efficiently get an "infinite" amount of subscriber.
However, this does put more issues in terms of idempotency to the system as what happens if a worker fails to create all of the jobs.
We should also consider caching topic calls pass as redis keys hold upto 512MB, however we are unsure if parsing that big of a document would be faster then calling mongodb
Why was this change needed?
Fixing this will allow large companies with large topics to send them messages without failure at the database level.
Definition of Done
References
https://medium.com/swlh/mongodb-pagination-fast-consistent-ece2a97070f3
novu/packages/application-generic/src/usecases/get-topic-subscribers/get-topic-subscribers.use-case.ts
Line 30 in 517b4fa
use keyset for the offset command, but must have the ids in order to do correctly