feat!: new event bus config format by rgraber · Pull Request #272 · openedx/openedx-events

rgraber · 2023-09-26T20:01:31Z

Description:
Change the event bus producing config from

{
    "event_type": [
        {topic: topic_a, event_key_field: field, enabled: True/False},
	{topic: topic_b, event_key_field: field, enabled: True/False},
    ]
}

to

{
     "event_type": {
         "topic_a":
	    {event_key_field: field, enabled: True/False},
	 "topic_b":
	    {event_key_field: field, enabled: True/False}
     }
}

ISSUE: #210

Dependencies: Would block openedx/openedx-platform#33269

Reviewers:

tag reviewer
tag reviewer

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.

robrap

I did not do a full review (yet?), but just noting that the ADR looks good. Thanks.

@navinkarkera: [question] Can we get away with the backward incompatible change? That would be simplest, but I didn't know if we needed temporary code that transforms, or the use of a new name for the setting etc. for your transition? Or is that all for named releases and will it be simple to transition with instructions?

bmtcril · 2023-09-29T16:43:08Z

This all makes sense to me. Since the original PR never landed on edx-platform I don't think there's any reason for any special shims or error handling for the transition.

navinkarkera · 2023-10-02T04:49:56Z

@robrap The original PR(s) using this config have not been merged yet, the one in edx-platform as well as openedx/xblock-skill-tagging#14. So we can go ahead with the backward incompatible change and update the PR's based on the decision and changes here.

navinkarkera

@rgraber The ADR looks good to me. 👍

robrap

A left a bunch of mostly non-blocking suggestions. Also, any could be added as another iteration if we wish to improve and you want to land this more quickly.

robrap · 2023-10-04T20:07:42Z

                raise ProducerConfigurationError(
                    event_type=event_type,
-                    message="One of the configuration object is not a dictionary"
+                    message="One of the configuration objects is not a dictionary"


Imagine everything is working fine, and someone adds a new bad config detail. Do we want that to break all working publishing? Seems scary. Something to consider.

Good point. I'm going to punt on this since it requires a little more thought than I want to put it while trying to land this but it will be a fast follow.

Isn't it better to fail early rather than allowing a back config detail? This error will stop the application from starting.

robrap

Thanks for the changes.

robrap

I think it is close.

robrap

Thank you.

navinkarkera

@rgraber This looks great. Feel free to ignore my suggestions if it doesn't make sense.

navinkarkera · 2023-10-06T13:46:36Z

+    for topic in event_type_producer_configs.keys():
+        if event_type_producer_configs[topic]["enabled"] is True:


Suggested change

for topic in event_type_producer_configs.keys():

if event_type_producer_configs[topic]["enabled"] is True:

for topic, config in event_type_producer_configs.items():

if config["enabled"] is True:

navinkarkera · 2023-10-06T13:47:27Z

-                topic=configuration["topic"],
-                event_key_field=configuration["event_key_field"],
+                topic=topic,
+                event_key_field=event_type_producer_configs[topic]["event_key_field"],


Suggested change

event_key_field=event_type_producer_configs[topic]["event_key_field"],

event_key_field=config["event_key_field"],

navinkarkera · 2023-10-06T13:48:50Z

                raise ProducerConfigurationError(
                    event_type=event_type,
-                    message="One of the configuration object is not a dictionary"
+                    message="One of the configuration objects is not a dictionary"


Isn't it better to fail early rather than allowing a back config detail? This error will stop the application from starting.

navinkarkera · 2023-10-06T13:51:54Z

    get_producer.cache_clear()
+
+
+def merge_producer_configs(producer_config_original, producer_config_overrides):


NIT: renaming these variables to original and overrides would make it easier to understand as producer_config prefix should not be needed.

Rebecca Graber added 5 commits September 29, 2023 08:13

feat!: new event bus config format

726f5eb

fixup!: ADR

688a72f

fixup!: ADR

28f5662

fixup!: ADR

ad615f0

fixup!: docs

d62ca05

rgraber force-pushed the rsgraber/20230926-update-publish-via-config branch from f752ddc to d62ca05 Compare September 29, 2023 12:13

fixup!: quality

bc0097c

robrap reviewed Sep 29, 2023

View reviewed changes

Comment thread docs/decisions/0013-new-event-bus-producer-config.rst Outdated

navinkarkera reviewed Oct 2, 2023

View reviewed changes

Comment thread docs/decisions/0013-new-event-bus-producer-config.rst Outdated

navinkarkera mentioned this pull request Oct 2, 2023

feat: replace signal handler with event producer config openedx/xblock-skill-tagging#14

Closed

10 tasks

Rebecca Graber and others added 11 commits October 4, 2023 08:31

fixup!: merge configs

90f8440

fixup!: merge configs

4340f98

fixup!: versioning

4c9578e

Merge branch 'main' into rsgraber/20230926-update-publish-via-config

5d7e609

fixup!: renumber

815bdf5

fixup!: docstring

c805931

fixup!: fix index

f01b55d

fixup!: quality

034d4d6

fixup!: fix test?

e36a22b

Merge branch 'main' into rsgraber/20230926-update-publish-via-config

07a8105

Merge branch 'main' into rsgraber/20230926-update-publish-via-config

36ba5dc

robrap reviewed Oct 4, 2023

View reviewed changes

Rebecca Graber added 2 commits October 5, 2023 08:34

fixup!: changes from review

c82fc3b

fixup!: space

1532ffb

rgraber commented Oct 5, 2023

View reviewed changes

Comment thread openedx_events/event_bus/__init__.py Outdated

fixup!: space

eb2d9ac

rgraber marked this pull request as ready for review October 5, 2023 12:41

rgraber requested a review from a team October 5, 2023 12:41

robrap reviewed Oct 5, 2023

View reviewed changes

Comment thread openedx_events/apps.py Outdated

Comment thread openedx_events/apps.py Outdated

Comment thread openedx_events/event_bus/__init__.py Outdated

Comment thread openedx_events/event_bus/__init__.py Outdated

Comment thread openedx_events/event_bus/__init__.py Outdated

fixup!: comments from review

537ea44

robrap reviewed Oct 5, 2023

View reviewed changes

Comment thread openedx_events/event_bus/__init__.py Outdated

robrap reviewed Oct 5, 2023

View reviewed changes

Comment thread docs/decisions/0014-new-event-bus-producer-config.rst Outdated

Rebecca Graber added 2 commits October 5, 2023 11:43

fixup!: comments from review

e25bff8

fixup!: comments from review

4157019

robrap mentioned this pull request Oct 5, 2023

Update event bus to use new producer configuration edx/edx-arch-experiments#381

Closed

3 tasks

robrap reviewed Oct 5, 2023

View reviewed changes

Comment thread docs/decisions/0014-new-event-bus-producer-config.rst Outdated

fixup!: comments from review

03e23b8

robrap approved these changes Oct 5, 2023

View reviewed changes

navinkarkera approved these changes Oct 6, 2023

View reviewed changes

rgraber merged commit 6e7dbd2 into main Oct 6, 2023

rgraber deleted the rsgraber/20230926-update-publish-via-config branch October 6, 2023 16:22

		for topic in event_type_producer_configs.keys():
		if event_type_producer_configs[topic]["enabled"] is True:

	event_key_field=event_type_producer_configs[topic]["event_key_field"],
	event_key_field=config["event_key_field"],

		get_producer.cache_clear()


		def merge_producer_configs(producer_config_original, producer_config_overrides):

Uh oh!

Conversation

rgraber commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmtcril commented Sep 29, 2023

Uh oh!

navinkarkera commented Oct 2, 2023

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rgraber commented Sep 26, 2023 •

edited

Loading