feat!: update event_bus_kafka and add event_bus_redis #3907
Conversation
openedx-webhooks
added
the
open-source-contribution
|
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:
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. |
| EVENT_BUS_PROCESSING_DELAY_SECONDS = 60 | ||
| EVENT_BUS_MESSAGE_DELAY_THRESHOLD_SECONDS = 60 |
There was a problem hiding this comment.
Changed names to make it generic
There was a problem hiding this comment.
[inform] I don't see any overrides in 2U's configuration repos, so this change should be safe to do without further coordination.
There was a problem hiding this comment.
...cancel that, there are references to the second one, EVENT_BUS_KAFKA_MESSAGE_DELAY_THRESHOLD_SECONDS. Repo owners will need to review and make some config changes in advance.
There was a problem hiding this comment.
Internal PR for updating these ahead of time: https://github.com/edx/edx-internal/pull/8376
There was a problem hiding this comment.
OK, the configs have been updated to use the new generic name, so we should be unblocked here.
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
1c971e9 to
e2c6a7d
Compare
navinkarkera
changed the title
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
e2c6a7d to
73f3f21
Compare
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
73f3f21 to
4cf1605
Compare
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
2 times, most recently
from
1580668 to
7739f32
Compare
|
Commit/PR title has typo "udpate"; should also use |
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
7739f32 to
c9d5c0b
Compare
navinkarkera
changed the title
|
@timmc-edx Updated the title. |
|
OK, just needs rebase or merge to resolve any conflicts with the master branch -- and someone from the owning team to review and approve. (Doesn't look like I can do it.) |
| EVENT_BUS_PROCESSING_DELAY_SECONDS = 60 | ||
| EVENT_BUS_MESSAGE_DELAY_THRESHOLD_SECONDS = 60 |
There was a problem hiding this comment.
So we're removing kafka config from edx-internal and keeping them only in Discovery? Are these settings not going to change?
There was a problem hiding this comment.
Nothing is being removed here, just renamed. The goal is to remove all Kafka references from the course-discovery source code so that deployers can choose to use event-bus-redis or other Event Bus implementations.
My understanding is that these delay settings aren't specific to Kafka, but are needed for any Event Bus implementation. I've already added the new name here, copying the value:
https://github.com/edx/edx-internal/pull/8376/files#diff-5e0513d8ace97560c1f17a81784c4d90cb5159626173cc2ffbc2e1fd05bd28b7R40 (private repo PR)
The next PR would remove the Kafka-specific name. The actual behavior shouldn't change at all.
| # edx-event-bus-kafka 3.7.0 introduces `reset_offsets_and_sleep_indefinitely` | ||
| edx-event-bus-kafka>=3.7.1 | ||
| # edx-event-bus-kafka 4.0.0 adds support for configurable consumer API | ||
| edx-event-bus-kafka>=4.0.1 |
There was a problem hiding this comment.
Can't we just remove this constraint? make upgrade will always find the latest version anyways.
There was a problem hiding this comment.
This is a pattern we've been following in some repos in order to prevent accidental regressions -- when we depend on a new API from some library, we bump the min-version constraint. We've occasionally seen unintended downgrades when a transitive dependency conflict occurs, and the pip constraint solver decides that the right answer is to downgrade some other dependencies. It's not essential to use this, though, and I'd be OK with removing it.
I'll also note that it's also a tool people sometimes use in order to do a "surgical upgrade" of one library, like the new make upgrade-package in edx-platform. (You bump the min-version constraint, then run make compile-requirements.) Arch-BOM is looking into how we can add upgrade-package to more repos, which would remove one of the reasons for setting a min-constraint.
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
c9d5c0b to
2c6ab4c
Compare
|
@timmc-edx Thanks, rebased with master. |
feat: add event bus redis as dependency refactor: add openedx_events in installed apps to use consume_events command chore: upgrade dependencies
navinkarkera
force-pushed
the
navin/event_bus_redis
branch
from
2c6ab4c to
1faa3c0
Compare
|
@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. |
Checklist