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

feat(bullmq): support prioritized job state #662

Merged
merged 1 commit into from
Sep 1, 2023
Merged

feat(bullmq): support prioritized job state #662

merged 1 commit into from
Sep 1, 2023

Conversation

patrickheeney
Copy link
Contributor

Changes Made

Added additional JobState recently introduced.
Reference: https://docs.bullmq.io/guide/jobs/prioritized
Reference: https://bullmq.io/news/062123/faster-priority-jobs/

Potential Risks

Tested on older bullmq and it ignores the prioritized state since it doesn't exist.

Test Plan

Screenshot 2023-08-20 at 8 31 45 PM

Checklist

  • I've increased test coverage
  • Since this is a public repository, I've checked I'm not publishing private data in the code, commit comments, or this PR.

@patrickheeney
Copy link
Contributor Author

Created patch file for patch-package.

patches/bull-arena+3.30.4.patch

diff --git a/node_modules/bull-arena/src/server/views/helpers/queueHelpers.js b/node_modules/bull-arena/src/server/views/helpers/queueHelpers.js
index 2e0f2e0..c37b56f 100644
--- a/node_modules/bull-arena/src/server/views/helpers/queueHelpers.js
+++ b/node_modules/bull-arena/src/server/views/helpers/queueHelpers.js
@@ -87,6 +87,7 @@ const Helpers = {
     'delayed',
     'paused',
     'waiting-children',
+    'prioritized',
   ],
 };

Copy link
Collaborator

@roggervalf roggervalf left a comment

Choose a reason for hiding this comment

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

as bullmq v3 does have this new state and if someone click on prioritized page from previous version, an error page will be shown. So or we treat this change as a breaking change or handle a way to check bullmq version to conditionally show this new state

@@ -87,6 +87,7 @@ const Helpers = {
'delayed',
'paused',
'waiting-children',
'prioritized',
Copy link
Collaborator

Choose a reason for hiding this comment

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

please move it after waiting state

@patrickheeney
Copy link
Contributor Author

patrickheeney commented Aug 23, 2023

an error page will be shown.

I am unable to replicate in my testing. Do you happen to have a screenshot of the error?

There shouldn't be any clickable link returned from jobCounts on older versions because arena does not pass any arguments to getJobCounts, thus bullmq uses its own internal list instead of the ones defined by arena which wouldn't include the new one.

I made the queueDetails logic match queueJobsByState which should resolve the issue.

@roggervalf
Copy link
Collaborator

I am unable to replicate in my testing. Do you happen to have a screenshot of the error?
hey @patrickheeney sure
Captura desde 2023-08-24 23-23-38

@roggervalf
Copy link
Collaborator

ok, I tried the last approach, but I prefer the ordering that was before and by adding prioritized state, I think it should appear in this order, people usually may look first for waiting states (waiting/prioritized), then active, completed and failed

'waiting' // first state that most jobs will start
'prioritized', // like waiting but with priority option, has less prevalence than waiting
'active', // after being in waiting or prioritized, it will be moved to active
'completed', // final state
'failed', // final state but with an error
'delayed', // it will be moved to waiting after some delay
'paused', // will not be moved until queue is resumed
'waiting-children' // if jobs are parents of other jobs

So for adding prioritized I may consider it as a breaking change in order to preserve this order and not confuse people using bullmq v3 with a broken page

@patrickheeney
Copy link
Contributor Author

Sorry, I am a little confused as to what you want me to do? Doesn't the order in the latest commit, match what you are requesting? Also there shouldn't be a 404 because it isn't returned from jobCounts in order to click on it, unless you are specifically typing in that URL or I am missing something?

@roggervalf
Copy link
Collaborator

hi @patrickheeney, I meant that the order is not respected with your last change, that was the purpose of BULLMQ_STATES to keep keep that order on our table

@patrickheeney
Copy link
Contributor Author

@roggervalf Sorry, I am still trying to understand.

You mentioned this about the order:
Screenshot 2023-08-31 at 7 46 04 PM

And this:
Screenshot 2023-08-31 at 7 46 21 PM

And this is the order of the last change:
Screenshot 2023-08-31 at 7 46 36 PM

Doesn't this match what you are suggesting? If not, can you please list them how you want it to be?

@@ -18,8 +18,6 @@ async function handler(req, res) {
if (queue.IS_BEE) {
jobCounts = await queue.checkHealth();
delete jobCounts.newestJob;
} else if (queue.IS_BULLMQ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, but after removing these lines the order changed

@roggervalf
Copy link
Collaborator

I'll fix it in another pr no worries

@roggervalf roggervalf merged commit 03475e7 into bee-queue:master Sep 1, 2023
4 checks passed
@beequeueci
Copy link
Collaborator

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants