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(job-scheduler): consider removing current job from wait, paused or prioritized #3066

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

roggervalf
Copy link
Collaborator

@roggervalf roggervalf commented Feb 8, 2025

Why

  1. Why is this change necessary? Before delayed job generates next one (when it's moved to active), it can be not only in delayed state, but also in wait, paused or prioritized.

How

  1. How did you implement this? Validate that if it's not in delayed, check prioritized, paused and wait state. To remove job and creates a new one when upserting

Additional Notes (Optional)

Any extra info here.

@roggervalf roggervalf changed the title fix(job-scheduler): consider removing current job from wait, paused o… fix(job-scheduler): consider removing current job from wait, paused or prioritized Feb 8, 2025
@roggervalf roggervalf force-pushed the fix-js-remove-current-job branch 7 times, most recently from e26d0fe to 196e055 Compare February 18, 2025 01:12
@roggervalf roggervalf force-pushed the fix-js-remove-current-job branch from 196e055 to b897552 Compare February 21, 2025 01:21
@roggervalf roggervalf force-pushed the fix-js-remove-current-job branch from b897552 to 5998c7a Compare February 21, 2025 05:33
Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

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

Just a small comment regarding the paused key.

@@ -4,6 +4,10 @@
Input:
KEYS[1] 'repeat' key
KEYS[2] 'delayed' key
KEYS[3] 'wait' key
KEYS[4] 'paused' key
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not have pause key anymore, so why is this key needed?

rcall("ZREM", prioritizedKey, currentJobId)
else
if isQueuePaused(KEYS[5]) then
if rcall("LREM", KEYS[4], 1, currentJobId) > 0 then
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed as there is no pause key.

Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

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

I am seeing that we actually still have the pause key even if it is not useful anymore. I guess we need to set it for deprecation then.

@roggervalf
Copy link
Collaborator Author

I am seeing that we actually still have the pause key even if it is not useful anymore. I guess we need to set it for deprecation then.

It's being removed as part of next breaking change #2707

@roggervalf roggervalf merged commit 97cd2b1 into master Feb 21, 2025
11 checks passed
@roggervalf roggervalf deleted the fix-js-remove-current-job branch February 21, 2025 13:55
github-actions bot pushed a commit that referenced this pull request Feb 21, 2025
## [5.41.5](v5.41.4...v5.41.5) (2025-02-21)

### Bug Fixes

* **job-scheduler:** consider removing current job from wait, paused or prioritized ([#3066](#3066)) ([97cd2b1](97cd2b1))
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.

2 participants