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

feat!: update event_bus_kafka and add event_bus_redis #3907

Merged
merged 1 commit into from
May 16, 2023
Merged

feat!: update event_bus_kafka and add event_bus_redis #3907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions course_discovery/apps/course_metadata/management/commands/consume_events.py
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions course_discovery/apps/course_metadata/signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,10 @@ def update_course_data_from_event(**kwargs):
if event_metadata:
event_timestamp = event_metadata.time
time_diff = datetime.now(tz=timezone.utc) - event_timestamp
if time_diff.seconds < settings.EVENT_BUS_KAFKA_MESSAGE_DELAY_THRESHOLD_SECONDS:
if time_diff.seconds < settings.EVENT_BUS_MESSAGE_DELAY_THRESHOLD_SECONDS:
logger.debug(f"COURSE_CATALOG_INFO_CHANGED event received within the delay "
f"applicable window for course run {course_data.course_key}.")
time.sleep(settings.EVENT_BUS_KAFKA_PROCESSING_DELAY_SECONDS)
time.sleep(settings.EVENT_BUS_PROCESSING_DELAY_SECONDS)

# Handle optional fields.
schedule_data = course_data.schedule_data
Expand Down
4 changes: 2 additions & 2 deletions course_discovery/apps/course_metadata/tests/test_signals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ def test_event_processing_delay(self, sleep_patch):
applicable time window.
"""
metadata = EventsMetadata(event_type='catalog-data-changed', minorversion=0)
with override_settings(EVENT_BUS_KAFKA_MESSAGE_DELAY_THRESHOLD_SECONDS=120):
with override_settings(EVENT_BUS_MESSAGE_DELAY_THRESHOLD_SECONDS=120):
with self.assertLogs(LOGGER_NAME, level="DEBUG") as logger:
update_course_data_from_event(
catalog_info=self.catalog_data,
Expand All @@ -874,4 +874,4 @@ def test_event_processing_delay(self, sleep_patch):
assert f"COURSE_CATALOG_INFO_CHANGED event received within the " \
f"delay applicable window for course run {self.course_key}." in logger.output[0]

sleep_patch.assert_called_once_with(settings.EVENT_BUS_KAFKA_PROCESSING_DELAY_SECONDS)
sleep_patch.assert_called_once_with(settings.EVENT_BUS_PROCESSING_DELAY_SECONDS)
5 changes: 3 additions & 2 deletions course_discovery/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
'taxonomy',
'django_object_actions',
'nested_admin',
'openedx_events',
]

ALGOLIA = {
Expand Down Expand Up @@ -721,8 +722,8 @@
},
}

EVENT_BUS_KAFKA_PROCESSING_DELAY_SECONDS = 60
EVENT_BUS_KAFKA_MESSAGE_DELAY_THRESHOLD_SECONDS = 60
EVENT_BUS_PROCESSING_DELAY_SECONDS = 60
EVENT_BUS_MESSAGE_DELAY_THRESHOLD_SECONDS = 60
Comment on lines +725 to +726
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed names to make it generic

Copy link
Contributor

Choose a reason for hiding this comment

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

[inform] I don't see any overrides in 2U's configuration repos, so this change should be safe to do without further coordination.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Internal PR for updating these ahead of time: https://github.com/edx/edx-internal/pull/8376

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, the configs have been updated to use the new generic name, so we should be unblocked here.

Comment on lines +725 to +726
Copy link
Contributor

Choose a reason for hiding this comment

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

So we're removing kafka config from edx-internal and keeping them only in Discovery? Are these settings not going to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


ALGOLIA_INDEX_EXCLUDED_SOURCES = []

Expand Down
5 changes: 5 additions & 0 deletions course_discovery/settings/devstack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@

CELERY_TASK_ALWAYS_EAGER = False

EVENT_BUS_CONSUMER = 'edx_event_bus_redis.RedisEventConsumer'
EVENT_BUS_PRODUCER = 'edx_event_bus_redis.create_producer'
EVENT_BUS_REDIS_CONNECTION_URL = 'redis://:[email protected]:6379/'
EVENT_BUS_TOPIC_PREFIX = 'dev'

#####################################################################
# Lastly, see if the developer has any local overrides.
if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')):
Expand Down
5 changes: 3 additions & 2 deletions requirements/base.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ edx-django-release-util
edx-django-sites-extensions
edx-django-utils
edx-drf-extensions
# edx-event-bus-kafka 3.7.0 introduces `reset_offsets_and_sleep_indefinitely`
edx-event-bus-kafka>=3.7.1
navinkarkera marked this conversation as resolved.
Show resolved Hide resolved
# edx-event-bus-kafka 4.0.0 adds support for configurable consumer API
edx-event-bus-kafka>=4.0.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just remove this constraint? make upgrade will always find the latest version anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

edx-event-bus-redis
edx-opaque-keys
edx-rest-api-client
elasticsearch
Expand Down
3 changes: 0 additions & 3 deletions requirements/constraints.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,3 @@ cairocffi < 1.5.0

# 1.0.0 has breaking changes
requests-toolbelt==0.10.1

# 4.0.0 introduces some breaking changes which will be resolved in a followup PR
edx-event-bus-kafka==3.9.6
21 changes: 15 additions & 6 deletions requirements/local.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ distro==1.8.0
# edx-django-utils
# edx-drf-extensions
# edx-event-bus-kafka
# edx-event-bus-redis
# edx-i18n-tools
# edx-toggles
# jsonfield
Expand Down Expand Up @@ -375,6 +376,7 @@ edx-django-utils==5.4.0
# django-config-models
# edx-drf-extensions
# edx-event-bus-kafka
# edx-event-bus-redis
# edx-rest-api-client
# edx-toggles
# getsmarter-api-clients
Expand All @@ -383,10 +385,10 @@ edx-drf-extensions==8.2.0
# via
# -c requirements/constraints.txt
# -r requirements/base.in
edx-event-bus-kafka==3.9.6
# via
# -c requirements/constraints.txt
# -r requirements/base.in
edx-event-bus-kafka==4.0.1
# via -r requirements/base.in
edx-event-bus-redis==0.1.1
# via -r requirements/base.in
edx-i18n-tools==0.9.2
# via -r requirements/local.in
edx-lint==5.3.4
Expand All @@ -403,7 +405,9 @@ edx-rest-api-client==5.5.0
# -r requirements/base.in
# taxonomy-connector
edx-toggles==5.0.0
# via edx-event-bus-kafka
# via
# edx-event-bus-kafka
# edx-event-bus-redis
elasticsearch==7.13.4
# via
# -c requirements/common_constraints.txt
Expand Down Expand Up @@ -556,6 +560,7 @@ openai==0.27.6
openedx-events==7.2.0
# via
# edx-event-bus-kafka
# edx-event-bus-redis
# taxonomy-connector
oscrypto==1.3.0
# via snowflake-connector-python
Expand Down Expand Up @@ -742,7 +747,9 @@ pyyaml==5.4.1
rcssmin==1.1.1
# via django-compressor
redis==4.5.5
# via -r requirements/base.in
# via
# -r requirements/base.in
# walrus
requests==2.30.0
# via
# -r requirements/base.in
Expand Down Expand Up @@ -954,6 +961,8 @@ vine==5.0.0
# kombu
virtualenv==20.23.0
# via tox
walrus==0.9.2
# via edx-event-bus-redis
wcmatch==8.4.1
# via semgrep
wcwidth==0.2.6
Expand Down
21 changes: 15 additions & 6 deletions requirements/production.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ django==3.2.19
# edx-django-utils
# edx-drf-extensions
# edx-event-bus-kafka
# edx-event-bus-redis
# edx-toggles
# jsonfield
# openedx-events
Expand Down Expand Up @@ -304,6 +305,7 @@ edx-django-utils==5.4.0
# django-config-models
# edx-drf-extensions
# edx-event-bus-kafka
# edx-event-bus-redis
# edx-rest-api-client
# edx-toggles
# getsmarter-api-clients
Expand All @@ -312,10 +314,10 @@ edx-drf-extensions==8.2.0
# via
# -c requirements/constraints.txt
# -r requirements/base.in
edx-event-bus-kafka==3.9.6
# via
# -c requirements/constraints.txt
# -r requirements/base.in
edx-event-bus-kafka==4.0.1
# via -r requirements/base.in
edx-event-bus-redis==0.1.1
# via -r requirements/base.in
edx-opaque-keys[django]==2.3.0
# via
# -r requirements/base.in
Expand All @@ -328,7 +330,9 @@ edx-rest-api-client==5.5.0
# -r requirements/base.in
# taxonomy-connector
edx-toggles==5.0.0
# via edx-event-bus-kafka
# via
# edx-event-bus-kafka
# edx-event-bus-redis
elasticsearch==7.13.4
# via
# -c requirements/common_constraints.txt
Expand Down Expand Up @@ -446,6 +450,7 @@ openai==0.27.6
openedx-events==7.2.0
# via
# edx-event-bus-kafka
# edx-event-bus-redis
# taxonomy-connector
oscrypto==1.3.0
# via snowflake-connector-python
Expand Down Expand Up @@ -547,7 +552,9 @@ pyyaml==6.0
rcssmin==1.1.1
# via django-compressor
redis==4.5.5
# via -r requirements/base.in
# via
# -r requirements/base.in
# walrus
requests==2.30.0
# via
# -r requirements/base.in
Expand Down Expand Up @@ -670,6 +677,8 @@ vine==5.0.0
# amqp
# celery
# kombu
walrus==0.9.2
# via edx-event-bus-redis
wcwidth==0.2.6
# via prompt-toolkit
webencodings==0.5.1
Expand Down