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(scheduler): remove multi when updating a job scheduler #3108

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

roggervalf
Copy link
Collaborator

Why

  1. Why is this change necessary? As part of giving support to serverless dbs

How

  1. How did you implement this? Move addition of delayed job when updating job scheduler

Additional Notes (Optional)

Any extra info here.

@roggervalf roggervalf force-pushed the update-scheduler branch 2 times, most recently from 74dd9b1 to a348c27 Compare February 28, 2025 05:07
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.

LGTM AFAICS, but I wrote a comment regarding a small change that seems to be a fix that would require a test case.

--- @include "isQueuePaused"
--- @include "storeJob"

local function addJobFromScheduler(jobKey, jobId, rawOpts, waitKey, pausedKey, metaKey, prioritizedKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

In time we will need to figure out some way to reduce the amount of arguments passed to this utility functions.

@@ -43,19 +51,16 @@ local prevMillis = rcall("ZSCORE", repeatKey, jobSchedulerId)
if prevMillis ~= false then
local currentDelayedJobId = "repeat:" .. jobSchedulerId .. ":" .. prevMillis

if producerId == currentDelayedJobId then
if producerId == currentDelayedJobId and rcall("EXISTS", nextDelayedJobKey) ~= 1 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 seems to be new code "and rcall("EXISTS", nextDelayedJobKey) ~= 1" not sure which issue it fixes but It would be nice with a test case that proves it is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually this checks is already in our currently job. When adding a delayed job in a separeted script, we check that it's not duplicated. Same logic is added here.

@roggervalf roggervalf merged commit 4b619ca into master Mar 11, 2025
11 of 12 checks passed
@roggervalf roggervalf deleted the update-scheduler branch March 11, 2025 13:59
github-actions bot pushed a commit that referenced this pull request Mar 11, 2025
## [5.41.9](v5.41.8...v5.41.9) (2025-03-11)

### Bug Fixes

* **scheduler:** remove multi when updating a job scheduler ([#3108](#3108)) ([4b619ca](4b619ca))
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