feat: utility to reset application state before processing event #277

navinkarkera · 2023-10-13T06:44:32Z

Description: Adds utility function to reset application state which can be used by event bus implementation to reset request/db_connection before processing events.

ISSUE: openedx/event-bus-redis#49

Related:

Reviewers:

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.

openedx-webhooks · 2023-10-13T06:44:38Z

Thanks for the pull request, @navinkarkera! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

felipemontoya · 2023-10-13T19:07:34Z

openedx_events/tooling.py

+    RequestCache.clear_all_namespaces()
+
+
+def prepare_for_new_work_cycle():


@navinkarkera looking at this PR I see that the new utility function is added but not executed anywhere.
If so, that means that it is the signal handler that must explicitly call this prepare_for_new_work_cycle function. Am I understanding the intent correctly?

@felipemontoya Yes, that is correct. Have created a draft PR: openedx/event-bus-redis#56 in event bus redis to use this utility.

This may be a good time to move https://openedx.atlassian.net/wiki/spaces/AC/pages/3662643205/How+to+add+a+new+concrete+implementation+of+the+event+bus to this repo and to update any incoming links to that doc to point to the newer version of the doc.

@robrap Done, we can update the incoming links once this PR is merged.

@navinkarkera: Were you still planning on updating the links?

navinkarkera · 2023-10-16T14:51:03Z

docs/how-tos/add-new-implementation.rst

+
+The consumer class then needs to implement ``consume_indefinitely`` loop, which will stay running and listen to events as they come in.
+
+If you are doing this within Django, one thing to be aware of is Django will not do any of the automatic connection cleanup that it usually does per request. This means that if your database restarts while the ``consume_indefinitely`` loop is running, Django may try to hold on to a defunct connection. To address this, we have included an `utility function <../../openedx_events/tooling.py#L323>`_ in openedx-events which needs to called before processing any signal. Checkout `consumer.py <https://github.com/openedx/event-bus-redis/blob/main/edx_event_bus_redis/internal/consumer.py>`_ in event bus redis implementation. We’re also resetting the RequestCache in the utility function, and there may be later, more comprehensive changes..


TODO: update links in follow up PR

robrap

Thanks for bringing in the doc.

docs/how-tos/add-new-implementation.rst

openedx_events/__init__.py

robrap · 2023-12-05T17:02:41Z

@navinkarkera: This PR has been sitting for a couple of months. Do you intend to return to this? It would be great to get it landed. Thank you!

navinkarkera · 2023-12-06T14:25:09Z

@robrap Apologies, I got caught up with some other projects. I'll surely get this done within this week.

navinkarkera · 2023-12-08T11:56:34Z

@robrap Thanks for your patience. Addressed your comments. Please have a look.

robrap · 2023-12-11T22:37:49Z

Thanks @navinkarkera and @mphilbrick211. This is on my list, but I've got a long list and lots of days off, so this may take me a little time to get to.

robrap

Thanks @navinkarkera. It looks like your fixup commit added requirements updates. Can we drop those changes from the PR, or do you need that for some reason? Other than that, this is ready to squash as-needed.

docs/how-tos/add-new-event-bus-concrete-implementation.rst

navinkarkera · 2024-01-03T14:25:51Z

It looks like your fixup commit added requirements updates. Can we drop those changes from the PR, or do you need that for some reason?

@robrap Actually, we added edx_django_utils to requirements as we need RequestCache for our new utility function.

robrap

Sorry @navinkarkera. Would you mind just updating the date? Then I'll merge. Thanks.

robrap · 2024-01-04T16:48:46Z

CHANGELOG.rst

@@ -14,6 +14,12 @@ Change Log
 Unreleased
 ----------

+[9.3.0] - 2023-12-08


Suggested change

[9.3.0] - 2023-12-08

[9.3.0] - 2024-01-04

@robrap Apologies for the delay. Updated the date.

robrap · 2024-02-02T22:25:22Z

@navinkarkera: Apologies, but I totally spaced on merging this, and now it has conflicts. I'm sorry about that. If we get it up-to-date net week I will merge as soon as you tell me it is ready. Thank you!

chore: bump version and update changelog chore: remove utility function from coverage docs: move how to add new implementation doc from atlassian docs: simplify consuming docs in how-to

openedx-webhooks · 2024-02-07T10:05:31Z

@navinkarkera 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

navinkarkera self-assigned this Oct 13, 2023

navinkarkera requested a review from a team as a code owner October 13, 2023 06:44

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 13, 2023

felipemontoya reviewed Oct 13, 2023

View reviewed changes

navinkarkera force-pushed the navin/work-cycle-utility branch 4 times, most recently from f91067d to 26e9aec Compare October 16, 2023 14:50

navinkarkera commented Oct 16, 2023

View reviewed changes

robrap reviewed Oct 16, 2023

View reviewed changes

docs/how-tos/add-new-implementation.rst Outdated Show resolved Hide resolved

docs/how-tos/add-new-implementation.rst Outdated Show resolved Hide resolved

docs/how-tos/add-new-implementation.rst Outdated Show resolved Hide resolved

openedx_events/__init__.py Outdated Show resolved Hide resolved

robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 5, 2023

navinkarkera force-pushed the navin/work-cycle-utility branch from 26e9aec to bafcb7a Compare December 8, 2023 11:55

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 11, 2023

mphilbrick211 requested a review from robrap December 11, 2023 18:28

robrap reviewed Jan 2, 2024

View reviewed changes

docs/how-tos/add-new-event-bus-concrete-implementation.rst Outdated Show resolved Hide resolved

navinkarkera force-pushed the navin/work-cycle-utility branch from bafcb7a to 4cf5488 Compare January 3, 2024 14:24

robrap approved these changes Jan 4, 2024

View reviewed changes

navinkarkera force-pushed the navin/work-cycle-utility branch from 4cf5488 to b7f7df1 Compare January 17, 2024 12:51

robrap added the Ready to Merge label Feb 2, 2024

feat: utility to reset application state before processing event

3fa34ad

chore: bump version and update changelog chore: remove utility function from coverage docs: move how to add new implementation doc from atlassian docs: simplify consuming docs in how-to

navinkarkera force-pushed the navin/work-cycle-utility branch from b7f7df1 to 3fa34ad Compare February 7, 2024 06:48

edx-requirements-bot merged commit 33cd7d5 into openedx:main Feb 7, 2024
8 checks passed

navinkarkera mentioned this pull request Feb 9, 2024

refactor: use utility from openedex_events to reset application … openedx/event-bus-redis#56

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: utility to reset application state before processing event #277

feat: utility to reset application state before processing event #277

navinkarkera commented Oct 13, 2023 •

edited

Loading

openedx-webhooks commented Oct 13, 2023 •

edited

Loading

felipemontoya Oct 13, 2023

navinkarkera Oct 16, 2023

robrap Oct 16, 2023

navinkarkera Oct 16, 2023

robrap Feb 9, 2024

navinkarkera Oct 16, 2023

robrap left a comment

robrap commented Dec 5, 2023

navinkarkera commented Dec 6, 2023

navinkarkera commented Dec 8, 2023

robrap commented Dec 11, 2023

robrap left a comment

navinkarkera commented Jan 3, 2024

robrap left a comment

robrap Jan 4, 2024

navinkarkera Jan 17, 2024

robrap commented Feb 2, 2024

openedx-webhooks commented Feb 7, 2024

		RequestCache.clear_all_namespaces()


		def prepare_for_new_work_cycle():


		The consumer class then needs to implement ``consume_indefinitely`` loop, which will stay running and listen to events as they come in.

		If you are doing this within Django, one thing to be aware of is Django will not do any of the automatic connection cleanup that it usually does per request. This means that if your database restarts while the ``consume_indefinitely`` loop is running, Django may try to hold on to a defunct connection. To address this, we have included an `utility function <../../openedx_events/tooling.py#L323>`_ in openedx-events which needs to called before processing any signal. Checkout `consumer.py <https://github.com/openedx/event-bus-redis/blob/main/edx_event_bus_redis/internal/consumer.py>`_ in event bus redis implementation. We’re also resetting the RequestCache in the utility function, and there may be later, more comprehensive changes..

feat: utility to reset application state before processing event #277

feat: utility to reset application state before processing event #277

Conversation

navinkarkera commented Oct 13, 2023 • edited Loading

openedx-webhooks commented Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

robrap commented Dec 5, 2023

navinkarkera commented Dec 6, 2023

navinkarkera commented Dec 8, 2023

robrap commented Dec 11, 2023

robrap left a comment

Choose a reason for hiding this comment

navinkarkera commented Jan 3, 2024

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap commented Feb 2, 2024

openedx-webhooks commented Feb 7, 2024

navinkarkera commented Oct 13, 2023 •

edited

Loading

openedx-webhooks commented Oct 13, 2023 •

edited

Loading