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): scheduled tasks does not persist on server restart #7959

Merged
merged 6 commits into from
Apr 19, 2021

Conversation

vkWeb
Copy link
Member

@vkWeb vkWeb commented Apr 6, 2021

Summary

On #7951 we changed the behaviour for the regular task queue to stay persistent on server restarts, but not for the scheduled tasks. This PR achieves persistency for scheduled tasks.

  • Now, we should be able to persist scheduled tasks across server restarts.
  • The schedule_pingback and schedule_vaccum functions should not queue in the scheduler if one already exists.

References

Closes #7954

Reviewer guidance

  1. Clear all the records of scheduled_jobs table in job_storage.sqlite3 db to avoid any confusion during testing.
  2. Start the server, now check the scheduled_jobs table, two jobs must be scheduled: _ping and vacuum.
  3. Schedule a new job.
  4. Restart the server, now all the scheduled jobs (_ping, vacuum and your own job) must have persisted.
  5. Also, make sure that _ping and vacuum aren't queued again on server restarts.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@vkWeb
Copy link
Member Author

vkWeb commented Apr 6, 2021

cc @rtibbles for review.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Can simplify approach and do some cleanup.

Would also be a good to add some tests of this behaviour.

kolibri/utils/server.py Outdated Show resolved Hide resolved
kolibri/core/tasks/scheduler.py Outdated Show resolved Hide resolved
@vkWeb
Copy link
Member Author

vkWeb commented Apr 17, 2021

@rtibbles sir approach has been simplified as you suggested. I'm new to mock so I'm learning it, tests will be pushed by tomorrow.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

This works as expected in local testing, and has the added benefit of not repeating the ping back on repeated restarts of the server - it persists the previous success and will ping again when the time is right!

@rtibbles rtibbles merged commit b3d89bd into learningequality:develop Apr 19, 2021
@vkWeb vkWeb deleted the fix/scheduler branch April 20, 2021 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants