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

fix(medusa): add support for retrying failed event bus jobs #2566

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

srindom
Copy link
Collaborator

@srindom srindom commented Nov 9, 2022

Closes CORE-770

@srindom srindom requested a review from a team as a code owner November 9, 2022 09:04
@changeset-bot
Copy link

changeset-bot bot commented Nov 9, 2022

🦋 Changeset detected

Latest commit: 71d6a11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM, added tiny suggestion to clean up options type

feel free to merge

@@ -192,9 +201,15 @@ export default class EventBusService {
})
return await stagedJobRepository.save(stagedJobInstance)
} else {
const opts: { removeOnComplete: boolean; delay?: number } = {
const opts: { removeOnComplete: boolean } & EmitOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

todo/suggestion(if-minor): Move removeOnComplete to EmitOptions and default value to options parameter instead of here

@kodiakhq kodiakhq bot merged commit 8069ed5 into develop Nov 9, 2022
@kodiakhq kodiakhq bot deleted the fix/retry-event-bus-jobs branch November 9, 2022 12:24
@github-actions github-actions bot mentioned this pull request Nov 9, 2022
@dwene
Copy link
Contributor

dwene commented Nov 14, 2022

Hmmm, I'm not sure this is the best for most of our use-cases with subscribers. I often have 5-6 subscribers on a single emit type (ex: order.placed). If any of the individual subscribers fail, based on the implementation I'm seeing here, they will all get re-run correct? So all the individual subscribers need to be smart enough to ignore retries if another subscriber failed.

Ex: If I send an email to the customer on order placed, but also have another subscriber (maybe from a plugin I installed) that continues to fail, the customer could receive lots of emails depending on the retry-policy, unless there is business logic that's checking to see if the email has already been sent?

In the past I've used medusa-extender to override the event-bus and add retries using try/catch in the worker instead of doing it the Bull way. That way I was only retrying the failed events and not spamming successful subscribers.

Just my 2 cents!

@srindom

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