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

[10.x] Add queue job debouncing #50347

Closed
wants to merge 12 commits into from
Closed

Conversation

nandi95
Copy link
Contributor

@nandi95 nandi95 commented Mar 2, 2024

Note

This is a work-in-progress, but I would like some feedback before spending any more time on this.

I have come across the need for such logic multiple times now, so I'm attempting to add this in a backward compatible way. The goal is to make it work with the existing infrastructure so the user doesn't have to add an extra trait or interface to the jobs, they can just start dispatching with debouncing working.

Why add this?

Sometimes a job might do some heavy computation or talk to an external api; things that the developer might decide is a waste of resources and should only be executed every so often. In this case, a debouncing pattern should be available to prevent spamming the logic path.

Example use-cases:

  • send a single email to user when an event happens or when a burst of events happens.
  • calculate some statistics after one or more resources changed in quick succession.
  • generate a report only once even when the user requests it repeatedly in a short period of time.

Questions/Considerations to discuss:

  • Redis count is atomic, what about the rest of the cache drivers?
  • How should sync driver handle this?
  • Any possible issues with batches and chained batches?
  • How could this be effectively tested?
  • Anything else?

Previous discussion: #43946

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

This approach doesn't work because it's possible the cache key is added but then the dispatch fails. This is why there is no such thing as exactly once delivery. Either we have at most once or at least once. Laravel always chooses at least once, but this deviates from that meaning that in some cases, jobs never get dispatched. I don't think this belongs in the core, but you could make a package for this.

@nandi95
Copy link
Contributor Author

nandi95 commented Mar 3, 2024

it's possible the cache key is added but then the dispatch fails

so if this is remedied, you would have no issues with it?
Could you please clarify what you mean by delivery?

Either we have at most once or at least once...

@driesvints driesvints marked this pull request as draft March 4, 2024 10:15
@driesvints
Copy link
Member

Please mark this as ready for review once you're done.

@nandi95
Copy link
Contributor Author

nandi95 commented Mar 4, 2024

The way I understand it the logic path of the failed dispatch is the following:

WorkCommand@handle -> WorkCommand@runWorker -> Worker@daemon -> Worker@runJob -> Worker@process -> Worker@handleJobException -> Worker@raiseExceptionOccurredJobEvent

This means we can register events against the JobExceptionOccurred. We could do this in the QueueServiceProvider
Please see the last commit doing this: 6a91cd1
Thoughts?

@driesvints
Copy link
Member

@nandi95 please just work out something you want to propose, make the tests pass and mark this PR as ready for review. Thanks!

@driesvints
Copy link
Member

Please send this to 11.x instead, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants