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

Enhanced job filtering, status change observers, runnable dependencies #30

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

robnewton
Copy link

This pull request includes various new features. It addresses issues with job filtering, model enhancements, and query filters. Additionally, it introduces configurable retries, minimum duration between attempts, and status change observers.

@iBotPeaches
Copy link
Member

Was this intended for your own fork? With all the namespace changes I can't tell if this was intended to upstream or not.

@robnewton
Copy link
Author

Oh I should have cleaned that up first sorry about that. Yeah we are using our fork in prod so changed the names so we could publish privately.

If you have any feedback outside of that, we can definitely clean that up before merging if it's something you think should be merged upstream.

@iBotPeaches
Copy link
Member

I do really like a few things

  • skip a job - useful for some of use cases
  • delete all failed jobs - useful for a use case

Others look good as well - just can't think at the moment of an immediate use case in our situations.

I think if you've been running these changes in prod on your side, I'll feel more comfortable upstreaming.

@robnewton
Copy link
Author

robnewton commented Jan 11, 2024

We are new to prod, so we can wait on a merge until a bit more time has passed. I just wanted to get it on your radar.

One key feature I can point out is the isRunnable() function added to job options.

When isRunnable() is defined, it allows RNQ to mark a job as skipped instead of failed when a dependency has not yet been met. Early in our testing, we saw jobs with network calls in them fail over and over when the device was offline, exhausting all configured retries almost immediately.

The isRunnable() function was devised to allow a job author to declare n dependencies by checking them in the function, then returning a boolean which indicates if all the jobs dependencies have been met. If RNQ receives a false from isRunnable() at runtime, the job is skipped with a skip reason added to the job object (similar to how failure reasons work).

Skipped jobs are then not processed and the worker function is not called. RNQ will then move onto the next job in the queue. The next time the queue runs (or the next batch of jobs fetched) the skipped jobs will be retried to see if isRunnable() returns true.

A couple of other experiments we wrote but ultimately stopped using once isRunnable() was implemented are the failureBehavior and minimumMillisBetweenAttempts. They were less than great solutions to the problem described above that I left in just incase they might be useful for someone else.

Happy to answer any questions.

@robnewton
Copy link
Author

Running in prod for a month or so now and the changes appear to be stable. No rush of course, just figured I'd shared the feedback.

@iBotPeaches
Copy link
Member

Thanks - I could consider upstreaming, but as the PR is now with all the branding changes/rename/etc. We couldn't merge that as-is.

@robnewton
Copy link
Author

Yeah, great point. I will try to put some cycles on a clean PR without the HopDrive stuff in it over the next month.

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

Successfully merging this pull request may close these issues.

3 participants