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: don't run cron on schedule #1139

Merged
merged 2 commits into from
Oct 22, 2024
Merged

fix: don't run cron on schedule #1139

merged 2 commits into from
Oct 22, 2024

Conversation

adityathebe
Copy link
Member

Need to explicitly start the cron to support multiple replicase (leader election).

This way multiple replicas can register the job without starting it and let the leader election start/stop the cron schedulers.

@adityathebe adityathebe marked this pull request as ready for review October 21, 2024 06:20
@moshloop
Copy link
Member

@adityathebe isn't this a breaking change? What happens if leader election is not supported/enabled ?

@adityathebe
Copy link
Member Author

@moshloop if not in election-leader mode, we'll have to call cron.Start() from canary-checker, config-db ,...

@moshloop
Copy link
Member

@adityathebe can we not make this backwards compatible - i.e. if leaderElectionEnabled {jobs.StartOnSchedule = false}

@adityathebe
Copy link
Member Author

@moshloop don't know if we should be to injecting any leader election stuff inside the job struct.

And I don't think backward compatibility is needed here.
I can merge this in and then block the automerge in other repos and merge with the fix.

need to explicitly start the cron to support multiple replicase (leader
election).

This way multiple replicas can register the job without starting it and
let the leader election start/stop the cron schedulers.
@moshloop moshloop merged commit 3228e40 into main Oct 22, 2024
7 checks passed
@moshloop moshloop deleted the fix/dont-run-cron branch October 22, 2024 06:04
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