Enforce a Celery singleton across cms and lms by using shared module #25840
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
feanil
approved these changes
Dec 10, 2020
|
With this, it would be safe to remove the conditional import in both |
This should prevent the issues we've seen recently where cms modules are imported by the running lms process, resulting in two celery instances being created and tasks intermittently being registered to the wrong instance (and therefore effectively lost.) In commit ab6bf34/PR #25822 we tried to ensure that only one or the other of the instances was created by adding a startup check. Unfortunately, there's an external shared library that refers directly to the lms celery, causing a startup failure in cms, so we had to revert it. Rather than waiting to fix that library, this commit collapses the two instances together so that there is only ever one.
timmc-edx
force-pushed
the
timmc/celery-highlander
branch
from
18dfff2 to
91f3428
Compare
timmc-edx
changed the title
feanil
approved these changes
Dec 10, 2020
|
Your PR has finished running tests. There were no failures. |
|
@nedbat this should be ported to Koa. @pomegranited FYI, we think this was the core problem with all the celery woes. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
|
Gosh, what a pain this was to locate! So glad you all manged to find and fix it! Thanks for letting me know, @feanil . |
nedbat
pushed a commit
that referenced
this pull request
Dec 11, 2020
…25840) This should prevent the issues we've seen recently where cms modules are imported by the running lms process, resulting in two celery instances being created and tasks intermittently being registered to the wrong instance (and therefore effectively lost.) In commit ab6bf34/PR #25822 we tried to ensure that only one or the other of the instances was created by adding a startup check. Unfortunately, there's an external shared library that refers directly to the lms celery, causing a startup failure in cms, so we had to revert it. Rather than waiting to fix that library, this commit collapses the two instances together so that there is only ever one. (cherry picked from commit 0c57a02)
|
I have cherry-picked this to Koa. |
This was referenced Dec 11, 2020
regisb
referenced
this pull request
in overhangio/tutor
Dec 12, 2020
regisb
referenced
this pull request
in overhangio/tutor
Dec 12, 2020
- [Bugfix] Fix missing celery tasks from edx-platform (see [upstream PR](https://github.com/edx/edx-platform/pull/25840))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should prevent the issues we've seen recently where cms modules are
imported by the running lms process, resulting in two celery instances
being created and tasks intermittently being registered to the wrong
instance (and therefore effectively lost.)
In commit ab6bf34/PR #25822 we tried to ensure that only one or the
other of the instances was created by adding a startup check.
Unfortunately, there's an external shared library that refers directly
to the lms celery, causing a startup failure in cms, so we had to revert
it. Rather than waiting to fix that library, this commit collapses
the two instances together so that there is only ever one.