-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: avoid infinite recursion of openedx-event #312
fix: avoid infinite recursion of openedx-event #312
Conversation
Thanks for the pull request, @Ian2012! 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:
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. |
0a0b857
to
ac0b58a
Compare
b18b09f
to
c6c3182
Compare
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.
I left a few no-blocking comments! Thank you
The next release with these changes will affect the event bus implementation tooling, so I'm pinging @robrap @timmc-edx so they know. Thanks! |
@mariajgrimaldi it will not affect them as |
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.
A few notes. Also remember to update version and changelog.
576e123
to
bbb2efe
Compare
@timmc-edx rebased and bumped. Do you think is ready to be merged? cc @mariajgrimaldi |
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.
One minor issue, but looks great overall!
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.
Looking good! Thanks. I'll merge after @Ian2012 addresses the last comment.
bbb2efe
to
133c299
Compare
chore: quality fixes fix: mark signals as already emitted by event bus automatically test: update test with from_event_bus chore: address suggestion test: implement tests chore: quality fixes chore: address suggestion test: correct tests chore: update debug log for signal processing from event bus Co-authored-by: Tim McCormack <[email protected]> chore: handle pr comments chore: handle pr comments
133c299
to
2891287
Compare
@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description: This PR adds a new argument
from_event_bus
to thesend_event
method (and related ones) that allows marking a Django signal as already processed from the event bus and avoids infinite emission of the same signal if such signal is consumed in the same runtime.The following code block must be used on the consumer to avoid processing the signal in the same process that emitted the signal:
This has been tested with the
event-bus-redis
without any changes as the behavior of callingsend_event_with_custom_metadata
is to enable thefrom_event_bus
param. Tested on openedx/event-routing-backends#361 with the signal emitted on openedx/event-tracking#246ISSUE: Solves: #79
Testing instructions:
Merge checklist:
Post merge:
finished.
Author concerns: List any concerns about this PR - inelegant
solutions, hacks, quick-and-dirty implementations, concerns about
migrations, etc.