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.

Sign up for GitHub

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

Jump to bottom

Prevent transient errors or network interruptions from interfering with syncs #11508

Merged
merged 7 commits into from
Nov 10, 2023
Merged

Prevent transient errors or network interruptions from interfering with syncs #11508

merged 7 commits into from
Nov 10, 2023

Conversation

bjester
Copy link
Member

@bjester bjester commented Nov 7, 2023 •
edited
Loading

Summary

  • Adds python decorator that will retry its wrapped function when a database is locked error occurs
  • Implements the above decorator for:
    • network discovery hooks to ensure the LOD request to sync doesn't stall from a locked error
    • the LOD sync hooks that update the sync queue status before and after a sync
  • Removes coach pluging sync hooks that triggered a cleanup of sync sessions when a LOD left the network, instead leveraging the result of Garbage Collection Integration for Morango SyncSessions #11101

References

Depends on: #11101
Resolves: #11485
Resolves: #11469

Reviewer guidance

  • Connect and disconnect Kolibris from a network
  • Use a flaky connection

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

@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: small labels Nov 7, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 7, 2023 •
edited
Loading

Build Artifacts

Asset type Download link
PEX file kolibri-0.16.0b7.dev0_git.38.g7ebbdf2b.pex
Windows Installer (EXE) kolibri-0.16.0b7.dev0+git.38.g7ebbdf2b-unsigned.exe
Debian Package kolibri_0.16.0b7.dev0+git.38.g7ebbdf2b-0ubuntu1_all.deb
Mac Installer (DMG) kolibri-0.16.0b7.dev0+git.38.g7ebbdf2b-0.3.0.dmg
Android Package (APK) kolibri-0.16.0b7.dev0+git.38.g7ebbdf2b-0.1.0-debug.apk
TAR file kolibri-0.16.0b7.dev0+git.38.g7ebbdf2b.tar.gz
WHL file kolibri-0.16.0b7.dev0+git.38.g7ebbdf2b-py2.py3-none-any.whl

@bjester bjester requested a review from rtibbles November 9, 2023 20:48
@bjester bjester marked this pull request as ready for review November 9, 2023 20:53
rtibbles
rtibbles reviewed Nov 9, 2023
View reviewed changes
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.

All makes sense to me. Nothing blocking.

kolibri/core/device/kolibri_plugin.py
from kolibri.plugins.hooks import register_hook


@register_hook
class SyncQueueStatusHook(FacilityDataSyncHook):
@retry_on_db_lock
Copy link
Member

Choose a reason for hiding this comment

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

Just to check my understanding - these hooks are executed outside the transaction for syncs right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that was the change we made a while back (and then re-did it)

kolibri/core/utils/lock.py Outdated Show resolved Hide resolved
kolibri/plugins/coach/kolibri_plugin.py
@@ -59,19 +57,3 @@ def plugin_data(self):
return {
"practice_quizzes_exist": practice_quizzes_exist,
}


@register_hook
Copy link
Member

Choose a reason for hiding this comment

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

This is just deferring all cleanup to the garbage collection, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, which I made the PR description say this depends on that PR

@marcellamaki marcellamaki self-requested a review November 10, 2023 21:59
marcellamaki
marcellamaki approved these changes Nov 10, 2023
View reviewed changes
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.

After a QA session with Blaine with a variety of scenarios, we will merge this. QA team will test as soon as we tag our next beta

@marcellamaki marcellamaki merged commit e2bf250 into learningequality:release-v0.16.x Nov 10, 2023
34 checks passed
@radinamatic radinamatic mentioned this pull request Nov 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rtibbles rtibbles rtibbles left review comments

@marcellamaki marcellamaki marcellamaki approved these changes

Assignees
No one assigned
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... SIZE: medium SIZE: small
Projects
None yet
Milestone
Kolibri 0.16.0: Bugs and Regressions
3 participants
@bjester @rtibbles @marcellamaki