-
Notifications
You must be signed in to change notification settings - Fork 21
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
[Abstraction] Determine a dependency versioning strategy for event-bus implementations #88
Comments
It turns out we have a collection of related dependency-related issues:
One thought I had is that we might just want to pack event-bus-kafka into openedx-events as a PEP-508 "extras" package, so we can include (You could even have a special CI workflow file on IDAs that confirms that none of the various extras' dependencies would conflict with the IDA's compiled requirements files.) |
@timmc-edx: As noted elsewhere, I don't know enough about "extras", but am hopeful that maybe this could help simplify things? I don't really know though. It might simplify/avoid some cases of expand-contract, but all implementations of the interface would need to be updated at once. And that would only work if we made a decision that all implementations must be "extras", and that no one could create implementations in other repos or beware of broken contracts. One point of clarification, is that I personally wouldn't refer to the relationship between and interface and its concrete implementation as a "weird sort-of-bidirectional dependency", unless I'm missing something. The concrete implementation is definitely dependent on the interface. The interface is not directly dependent on the implementation, but it needs to know and respect that it is a contract with a set of unknown (or possibly known) consumers. I think this just comes with the territory of loose coupling. It can certainly make things messy or interesting, which might be what you meant by "weird"? I'm simply noting that it is expected, and I don't think we are doing anything out of the ordinary. |
What's feeling weird about it is that the implementation has a package-dependency on the interface package, but the interface package loads the implementation. But... just a feeling. I haven't thought it all through yet, though, so maybe there's no actual additional complication resulting from that. |
Could you please point me to where that loading happens? Does it do something other than receive Django signals?
I don't think I fully understand the issues at play here, but would it simplify things any to always install the kafka dependencies? We could keep the entire package separated at the top level (so it'd be in its own package as a peer to |
The idea is for This also entails having a way for anyone to privately install the Kafka dependency, which for now was being done in our private Dockerfile. As you point out, a quick solution would be to simply install the Kafka dependencies for everyone. This could be done now by adding our Kafka dependency (event-bus-kafka) in the requirements files of the producing or consuming service. The Kafka code could continue to live in a separate repo. This would be a decision to temporarily punt this issue down the road. |
@felipemontoya and @mariajgrimaldi should weigh in, but I'm personally okay with Kafka dependencies directly in this repo in the short term to get the first iteration out the door, so long as it's very clear how to separate it into a new repo later (e.g. different top level package). Please forgive my ignorance here, but what common APIs are necessary across the different bus backends aside from sending/receiving signals? |
Thanks @ormsbee.
|
@timmc-edx: If we decide to simply make Kafka a dependency, does adding |
I'd like to make sure I understand what adding the kafka dependency here means. Is it to simply add If it is the first I would personally not mind it either. If the "few others" are all the dependencies from the event-bus-kafka I suppose that would be ok to get this off the pad but in that case I would also advocate for using the extras to keep the package lightweight for other uses in plugin development. |
@felipemontoya: See my responses above. I am proposing that nothing changes for openedx-events in the short term. |
Extra dependencies, in this case, seem quite handy. But it seems like a good idea if there's some urgency in the issue to include the dependency in the requirements files of the producing or consuming service. I still wonder how to handle conflicting versions of |
@robrap Adding There are still some issues to resolve around how we'll coordinate API change management, but I think we can figure that out as needed. (We can just do major-version bumps of openedx-events and event-bus-kafka in lockstep, and if that's too rigid we can do something more clever.) |
Adding the dependencies in the service also means we can use the constraints file to manage rollouts as required. |
As per openedx/openedx-events#88 we're going to try explicit dependencies on implementations for now, rather than solve all the problems we'd encounter by using private dependencies.
We had an issue earlier where This has been discussed with the confluent devs: confluentinc/confluent-kafka-python#1405 There has been a wheel published for M1 Macs/ARM64: https://github.com/confluentinc/confluent-kafka-python/releases/tag/v1.9.2 |
Implements #30682 Produce signal only once transaction for a course publish is committed, and only for actual courses (not libraries). - Use newer openedx-events version that has a fix for None datetime and that has CourseCatalogData without org, number. - Add edx-event-bus-kafka -- specify recent version that drops confluent-kafka from explicit deps - New functionality behind toggle As per openedx/openedx-events#88 we're going to try explicit dependencies on implementations for now, rather than solve all the problems we'd encounter by using private dependencies.
Implements #30682 Produce signal only once transaction for a course publish is committed, and only for actual courses (not libraries). - Use newer openedx-events version that has a fix for None datetime and that has CourseCatalogData without org, number. - Add edx-event-bus-kafka -- specify recent version that drops confluent-kafka from explicit deps - New functionality behind toggle As per openedx/openedx-events#88 we're going to try explicit dependencies on implementations for now, rather than solve all the problems we'd encounter by using private dependencies.
Implements #30682 Produce signal only once transaction for a course publish is committed, and only for actual courses (not libraries). - Use newer openedx-events version that has a fix for None datetime and that has CourseCatalogData without org, number. - Add edx-event-bus-kafka -- specify recent version that drops confluent-kafka from explicit deps, fixes common auth settings, and has a multi-producer caching tweak. - New functionality is behind toggle As per openedx/openedx-events#88 we're going to try explicit dependencies on implementations for now, rather than solve all the problems we'd encounter by using private dependencies. Co-authored-by: Tim McCormack <tmccormack@edx.org> Co-authored-by: Rebecca Graber <rgraber@edx.org>
I think what we want at this point is just an ADR in openedx-events saying that we're going to depend on all Event Bus implementations for now and will figure out a better approach later, if needed. (event-bus-kafka no longer has a hard dependency on confluent-kafka as far as pip is concerned: https://github.com/openedx/event-bus-kafka/blob/main/docs/decisions/0005-optional-import-of-confluent-kafka.rst) |
I'm actually confused about the current status. That ADR does not have what I remember. :) I thought we were getting upgrades via make upgrade, but I don't see where we are requiring it other than tests, and the weird enterprise constraint. See https://github.com/search?p=1&q=org%3Aopenedx+confluent-kafka&type=Code.
|
Currently it is pulled in here, in event-bus-kafka test.in -- but not by the package's base.in: https://github.com/openedx/event-bus-kafka/blob/701098b0b7a671982b8c74855afd13c04ef81612/requirements/test.in#L9 (note that it's spelled with an underscore here, which is why it wasn't coming up in your search -- we should probably switch it to a hyphen to match its canonical spelling!) So event-bus-kafka doesn't pull it in as a dependency, but will just be a no-op if you try to use it. CMS and course discovery have to install confluent-kafka in their private dockerfile or something. |
The part I'm confused about is I thought we determined that upgrades weren't going to be issue because we were using a public dependency from the services and So this ticket, or something related to this ticket, still seems like it applies, right? |
At the time this ticket was written, |
Created edx/edx-arch-experiments#351 for the general case of "we don't know how to manage plugin versions yet". |
The
edx_event_bus_kafka
dependency will be specified and pinned in a file outside of each IDA that uses it, since openedx-events will load the implementation by looking up a module reference in the IDA's config. This means that dependency version changes on the library will not be versioned with the IDA (i.e. in the requirements/base.txt files) and we'll need some sort of expand-contract approach to API changes.For example, we might need to do something like this:
KafkaProducer
class in event-bus-kafka and delegate the existingsend_to_event_bus
to use itsend_to_event_bus
function from event-bus-kafkaEvery master-branch deployer would need to do this in lockstep. (Maybe that's just 2U?) For Open edX releases we might be able to get away with just saying "use the release tag".
Acceptance Criteria
The text was updated successfully, but these errors were encountered: