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.

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: Configurable loading of event bus producer #85

Merged
merged 3 commits into from
Nov 22, 2022
Merged

Conversation

timmc-edx
Copy link
Contributor

@timmc-edx timmc-edx commented Jul 28, 2022

This defines an API for IDAs to use when producing events that allows the
choice of Event Bus implementation to be configured rather than written in
code. Implementations can document the appropriate setting for deployers
to use.

Caching the producer instance means that we automatically get a
long-lived instance without the implementation itself having to
perform caching itself.

This is part of #87

Merge checklist:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Generally right direction. Just digging into details.

openedx_events/event_bus/__init__.py Outdated Show resolved Hide resolved
openedx_events/event_bus/__init__.py Outdated Show resolved Hide resolved
"""

@abstractmethod
def send(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming that this replaces the previously documented send_to_event_bus, since this would probably result in code like event_bus.send(...)?
Related, do we like using the term send rather than produce, to provide overlap with the signals, since this takes a signal? Or would something like produce_from_signal be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion! But yeah, it seemed redundant to use the existing name.

openedx_events/event_bus/__init__.py Show resolved Hide resolved
openedx_events/event_bus/__init__.py Outdated Show resolved Hide resolved
@mphilbrick211
Copy link

Hi @timmc-edx! Just checking to see if you are planning to proceed with this PR?

@timmc-edx
Copy link
Contributor Author

Yep! This work was deprioritized and has bit-rotted a bit, but I'll be revisiting it.

@robrap
Copy link
Contributor

robrap commented Oct 26, 2022

@timmc-edx: We have #87 which mentions wip PRs. You are welcome to close the PRs and verify the issue would help someone find them when and if the work is picked up. That said, do whatever feels right.

@timmc-edx timmc-edx force-pushed the timmc/loader branch 2 times, most recently from 66ffb75 to 5af0278 Compare November 4, 2022 22:06
@timmc-edx
Copy link
Contributor Author

timmc-edx commented Nov 4, 2022

I pushed up something of a rewrite of the loader code. It may be that some of it should get pushed into an internal module, but I think this will do what we need and allow us to remove Kafka references from IDAs.

@timmc-edx timmc-edx marked this pull request as draft November 7, 2022 15:29
@timmc-edx timmc-edx marked this pull request as ready for review November 17, 2022 20:42
@timmc-edx
Copy link
Contributor Author

This is ready for review again, and is accompanied by openedx/event-bus-kafka#14

I'm a little unsure about what the consumer API should look like, and it might be better to move that to a new PR later. It might be better to just focus on the producer part right now.

@robrap robrap added the event-bus Work related to the Event Bus. label Nov 18, 2022
@robrap
Copy link
Contributor

robrap commented Nov 18, 2022

@timmc-edx: I think we decided we would close/defer this? We could create a separate issue, or just use this PR as the issue, if you are ok with long-standing PRs. We should add backlog label and move to Future milestone, if you agree with status. Thanks!

This defines an API for IDAs to use when producing events that allows the
choice of Event Bus implementation to be configured rather than written in
code. Implementations can document the appropriate setting for deployers
to use.

Caching the producer instance means that we automatically get a
long-lived instance without the implementation itself having to
perform caching itself.
@timmc-edx timmc-edx changed the title feat: Configurable loading of event bus; API for calling it feat: Configurable loading of event bus producer Nov 18, 2022
@timmc-edx
Copy link
Contributor Author

This one is just for loading of the implementation -- and for now, scoped down to just the producing side. I'm moving the consumer side into a separate branch for later consideration.

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

I think this is worth a small how-to with examples of the kinds of things you might put in the EVENT_BUS_PRODUCER setting. The how-to could basically copy the setting description but I think a how-to would be a more accessible place for it.

openedx_events/event_bus/__init__.py Outdated Show resolved Hide resolved
openedx_events/event_bus/__init__.py Outdated Show resolved Hide resolved
Remove use of `UserWarning`, as it is the default.
@timmc-edx
Copy link
Contributor Author

@rgraber Hmm. I feel like it wouldn't be that helpful to give an example of the setting without backing code. The producer setting already gives an example of some.module.path.EventBusImplementation but of course there's no EventBusImplementation to look at. Perhaps the documentation could instead link to event-bus-kafka's documentation as an example? It's all a bit chicken-and-egg, but I could come back and make a new PR here once https://github.com/openedx/event-bus-kafka/pull/14/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168f is merged.

@rgraber
Copy link
Contributor

rgraber commented Nov 22, 2022

@rgraber Hmm. I feel like it wouldn't be that helpful to give an example of the setting without backing code. The producer setting already gives an example of some.module.path.EventBusImplementation but of course there's no EventBusImplementation to look at. Perhaps the documentation could instead link to event-bus-kafka's documentation as an example? It's all a bit chicken-and-egg, but I could come back and make a new PR here once https://github.com/openedx/event-bus-kafka/pull/14/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168f is merged.

I think coming back once that PR is merged makes sense

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

LGTM

@timmc-edx
Copy link
Contributor Author

OK, I'll add a ticky box for that on the issue.

@timmc-edx timmc-edx merged commit 2959be7 into main Nov 22, 2022
@timmc-edx timmc-edx deleted the timmc/loader branch November 22, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event-bus Work related to the Event Bus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants