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

Task scheduling updates for Android #11480

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 31, 2023

Summary

  • Moves the marking of a job as running inside the job execute method so that it happens independent of task runner
  • Make the rescheduling of a task after it has completed (either for repeat or retry behaviour) separate from the updating of the status
  • Most importantly, uses the schedule method to reschedule the new task, rather than the update methods, so that schedule hooks are called
  • Adds some reusable validation functions for commonly used scheduling arguments
  • Move Priority enum to constants to avoid a circular import
  • Fixes unnoticed bug in enqueue_if_not that failed to pass through passed in arguments.
  • Updates how retry_in works to defer setting until after the job has completed, and then passing the new arguments to the reschedule job - allows the scheduling to be deferred until task completion, and to allow the retry_in behaviour to be used more liberally

References

Should resolve Android syncing issues that were caused by the combination of the existing retry_in behaviour (which set repeat to 1) and the update hooks that pass the orm_job to the update hooks after the orm_job has been modified (so repeat would be 0, and no repeat would be scheduled by the Android hook).

Does this by making sure that all job scheduling events trigger the schedule hook, and that the update hook is only for processing state updates. This feels like it makes a lot more sense anyway!

Reviewer guidance

The APK generated here will probably mostly work as it is - but I have some correlated changes for Android which I will open in a PR on the Android repo. I will also build an APK from this PR with those changes for testing.

Further testing on Android has shown some issues, but not related to this PR, it seems to have uncovered some weirdness that I hadn't previously observed in properly reading the job_id within the WorkManager context. This PR can be reviewed independently.

@pcenov and @radinamatic please make sure that these changes cause no regressions in existing syncing behaviour.


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

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 31, 2023
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: large labels Oct 31, 2023
@rtibbles rtibbles force-pushed the task_scheduling_updates branch from b572743 to 84cd620 Compare October 31, 2023 03:26
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @rtibbles - no regressions observed in the existing syncing behaviour while manually testing today on several devices.
The Android app behaviour is also unchanged (not syncing after the initial sync) - but as you mentioned this will be tested separately when you prepare a new Android build for that.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

code read through looks okay to me

@marcellamaki marcellamaki merged commit a9289de into learningequality:release-v0.16.x Oct 31, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: large TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants