-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: Reset request cache before handling event #201
Conversation
This is a just-in-case measure -- we don't know of any specific code that handles events and relies on the cache. But it's likely to come up at some point. Ticket: openedx/openedx-events#235
# Before processing, make sure our db connection is still active | ||
_reconnect_to_db_if_needed() | ||
# Before processing, make sure our application state is similar to | ||
# that of a new Django request. (Mimic setup/teardown.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking question: By "setup/teardown", are you referring to something related to Django requests, or just unit tests or some such? If Django requests, I have openedx/openedx-events#236 open, because I'm wondering if there isn't a more general solution to this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Django requests. I wonder if it would make sense to link to that issue to give context that "mimic setup/teardown" is more aspirational than concrete. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It wouldn't hurt to link to that issue and add some clarity.
- It actually feels like
prepare_for_new_work_cycle
probably belongs in openedx-events, which would give this benefit to Redis for free. Maybe we could create an issue for that, and let the Redis team know about it. In place of copying this change, maybe they can implement a more shareable solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created openedx/openedx-events#270
This is a just-in-case measure -- we don't know of any specific code that handles events and relies on the cache. But it's likely to come up at some point.
Ticket: openedx/openedx-events#235
Merge checklist:
Check off if complete or not applicable: