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

feat: add batching for event bus consumer #388

Merged
merged 8 commits into from
Mar 22, 2024
Merged

feat: add batching for event bus consumer #388

merged 8 commits into from
Mar 22, 2024

Conversation

Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Feb 2, 2024 •
edited
Loading

Description

This PR adds batching for the event bus consumer based on a Redis list and creates a queue to store failed bulk events.

Author concerns

The dead queue will grow indefinitely when the xAPI backend is down for too long. We should add a setting to control the maximum size of the dead queue.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 2, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Feb 2, 2024 •
edited
Loading

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:

  • 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.

@Ian2012 Ian2012 force-pushed the cag/add-consumer-for-tracking-logs branch from e3e7df9 to f138ce5 Compare February 2, 2024 19:14
@Ian2012 Ian2012 changed the title tmp: add fake redis feat: add batching for event bus consumer Feb 2, 2024
@Ian2012 Ian2012 force-pushed the cag/add-consumer-for-tracking-logs branch 3 times, most recently from cb0476d to 14df49d Compare February 2, 2024 19:22
@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch 4 times, most recently from 2c13d4a to b2553c1 Compare February 5, 2024 23:58
@Ian2012 Ian2012 marked this pull request as ready for review February 5, 2024 23:58
@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch 2 times, most recently from 5dc5338 to 27f0529 Compare February 6, 2024 14:38
@Ian2012 Ian2012 force-pushed the cag/add-consumer-for-tracking-logs branch from ed0cde7 to 6e2acb1 Compare February 13, 2024 17:12
@bmtcril bmtcril mentioned this pull request Feb 13, 2024
Closed
3 tasks
@Ian2012 Ian2012 force-pushed the cag/add-consumer-for-tracking-logs branch 5 times, most recently from 3a21781 to 9d5e704 Compare February 16, 2024 21:00
Base automatically changed from cag/add-consumer-for-tracking-logs to master February 19, 2024 16:32
@bmtcril
Copy link
Contributor

bmtcril commented Feb 21, 2024

@Ian2012 do you want to rebase this one and we can review?

@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch 2 times, most recently from 473f3d3 to 2d4c172 Compare February 22, 2024 15:08
@Ian2012 Ian2012 requested a review from bmtcril February 22, 2024 15:08
@Ian2012
Copy link
Contributor Author

Ian2012 commented Feb 22, 2024 •
edited
Loading

@bmtcril Updated the implementation to use the EventRouter as the batcher. It implemented a setting to enable batching and configure the batch size. When tested with celery there are fewer tasks in the queue and the task times are improved because the tracking logs are not processed individually anymore

@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch from 2d4c172 to 87d7252 Compare February 22, 2024 15:18
@Ian2012 Ian2012 marked this pull request as draft February 26, 2024 19:27
@Ian2012 Ian2012 marked this pull request as ready for review March 1, 2024 14:59
@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch 2 times, most recently from 3a8032c to a8566e1 Compare March 1, 2024 15:54
@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch 3 times, most recently from 17afff7 to ca17c49 Compare March 1, 2024 17:21
Ian2012 added 2 commits March 1, 2024 12:31
@Ian2012
feat: send events in batch from routers
f6bb6ae 
fix: add time_to_send method

chore: quality fixes
@Ian2012
chore: bump version to 8.2.0
cfee7da
@Ian2012 Ian2012 force-pushed the cag/add-batching-for-consumer branch from ca17c49 to cfee7da Compare March 1, 2024 17:31
@bmtcril bmtcril requested review from ziafazal and pomegranited March 1, 2024 17:47
pomegranited
pomegranited approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

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

👍 I haven't set up the event bus, but the batching works with a non-bus setup.

Just had a couple comments inline, but they don't have to block merging here.

  • I tested this on my tutor dev stack with batching enabled. Events moved through the pipeline as expected.
  • I read through the code
  • I checked for accessibility issues N/A
  • Includes documentation
  • User-facing strings are extracted for translation N/A

event_routing_backends/settings/common.py Outdated Show resolved Hide resolved
event_routing_backends/settings/common.py Show resolved Hide resolved
test_settings.py Outdated Show resolved Hide resolved
pylintrc Outdated Show resolved Hide resolved
@Ian2012 Ian2012 mentioned this pull request Mar 20, 2024
Merged
8 tasks
@Ian2012
Copy link
Contributor Author

Ian2012 commented Mar 20, 2024

Hi @ziafazal, do you have any suggestions here?

@bmtcril this one is ready for review. The dead letter queue has been implemented

bmtcril
bmtcril reviewed Mar 20, 2024
View reviewed changes
event_routing_backends/backends/events_router.py Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
docs/getting_started.rst Outdated Show resolved Hide resolved
event_routing_backends/management/commands/recover_failed_events.py Outdated Show resolved Hide resolved
event_routing_backends/management/commands/recover_failed_events.py Show resolved Hide resolved
test_settings.py Show resolved Hide resolved
bmtcril
bmtcril approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

LGTM thanks for addressing all of that!

@Ian2012 Ian2012 merged commit 2b31649 into master Mar 22, 2024
10 checks passed
@Ian2012 Ian2012 deleted the cag/add-batching-for-consumer branch March 22, 2024 19:39
@openedx-webhooks
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipemontoya felipemontoya felipemontoya left review comments

@bmtcril bmtcril bmtcril approved these changes

@pomegranited pomegranited pomegranited approved these changes

@ziafazal ziafazal Awaiting requested review from ziafazal

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ian2012 @openedx-webhooks @bmtcril @felipemontoya @pomegranited