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

Separate morango pull/push clients with better write locking and better cancellation handling #7251

Merged
merged 12 commits into from
Jul 15, 2020

Conversation

bjester
Copy link
Member

@bjester bjester commented Jul 6, 2020

Summary

This PR integrates the latest morango changes in this PR, and only uses the db_write_task_lock when data is queuing or dequeuing locally. This also attempts to address cancellation during the stages of a sync task, since we should only allow cancellation when we can expect that the cancellation will occur as expected. For instance, attempting to cancel during a remote dequeuing shouldn't be allowed, as we don't have that control over the remote's processing.

Also addresses the issues with blank passwords as shown in #7127 by setting any blank passwords to NOT_SPECIFIED when deserializing.

No cancel button shown when locally integrating received data, or aka local dequeuing. Also progress is indeterminate.
Screenshot from 2020-07-07 14-17-57

Reviewer guidance

  • Utilize the UI to schedule a sync
  • Use the management command to sync a facility
  • Ensure cancelling during transfer works as expected
  • Cancel button shouldn't appear when the task is in a uncancellable stage

References

Depends on: learningequality/morango#85
Related: #7125
Resolves: #7127


Contributor Checklist

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

Testing:

  • 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

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

@bjester bjester added the work-in-progress Not ready for review label Jul 6, 2020
@bjester bjester added this to the 0.14.0 milestone Jul 6, 2020
@bjester bjester added DEV: backend Python, databases, networking, filesystem... python labels Jul 7, 2020
@bjester bjester linked an issue Jul 7, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #7251 into release-v0.14.x will increase coverage by 0.08%.
The diff coverage is 24.00%.

Impacted Files Coverage Δ
kolibri/core/auth/management/commands/sync.py 0.00% <0.00%> (ø)
kolibri/core/tasks/worker.py 84.07% <ø> (ø)
kolibri/core/auth/models.py 91.26% <80.00%> (-0.09%) ⬇️
kolibri/core/tasks/api.py 44.12% <100.00%> (+0.27%) ⬆️
kolibri/core/tasks/job.py 78.12% <100.00%> (+1.98%) ⬆️
kolibri/core/tasks/storage.py 86.23% <100.00%> (+0.20%) ⬆️
...i/plugins/device/assets/src/views/syncTaskUtils.js 65.38% <100.00%> (+0.67%) ⬆️
...bri/core/content/utils/importability_annotation.py 50.80% <0.00%> (-13.12%) ⬇️
kolibri/core/auth/middleware.py 88.37% <0.00%> (-11.63%) ⬇️
kolibri/core/content/models.py 82.35% <0.00%> (-5.15%) ⬇️
... and 56 more

@bjester bjester changed the title Integrate morango updates into sync command, cancellation updates Separate morango pull/push clients with better write locking and better cancellation handling Jul 8, 2020
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.

Could simplify some of this slightly by saving the change in cancellable. This will need a bit of enhancement of the job API to add a save method, so I think we can skip this for now, and clean this up in later refactor.

Overall approach looks sound.

kolibri/core/auth/management/commands/sync.py Show resolved Hide resolved
kolibri/core/auth/management/commands/sync.py Outdated Show resolved Hide resolved
kolibri/core/tasks/api.py Outdated Show resolved Hide resolved
@bjester bjester marked this pull request as ready for review July 14, 2020 23:58
@bjester bjester requested a review from rtibbles July 14, 2020 23:59
@bjester bjester added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Jul 14, 2020
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.

Looks good to me - have not done any manual testing, but the code changes look right. Let's merge this to do bug bashing on it.

@rtibbles rtibbles merged commit 6a9159e into learningequality:release-v0.14.x Jul 15, 2020
@jonboiser jonboiser removed the TODO: needs review Waiting for review label Jul 20, 2020
@bjester bjester deleted the step-by-sync branch July 15, 2021 17:43
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...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issues with facility import functionality
3 participants