Skip to content

Commit

Permalink
fixup!: ADR
Browse files Browse the repository at this point in the history
  • Loading branch information
rgraber committed Aug 17, 2022
1 parent bd780d7 commit 8d5347e
Showing 1 changed file with 16 additions and 16 deletions.
32 changes: 16 additions & 16 deletions docs/decisions/0005-conditional-import-of-confluent-kafka.rst
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
4. Conditional import of confluent-kafka
=======================================
4. Optional import of confluent-kafka
====================================

Status
------
Expand All @@ -9,35 +9,35 @@ Accepted
Context
-------

* confluent-kafka is a library written and distributed by Confluent, our managed instance provider. It abstracts out
the work for sending and receiving events to and from the Kafka cluster and converting them into message objects.
* confluent-kafka relies on a C library called librdkafka-dev
* `confluent-kafka`_ is a library written and maintained by Confluent, our managed instance provider (see :doc:`0004-kafka-managed-hosting`). The library abstracts out the work for sending and receiving events to and from the Kafka cluster and converting them into message objects.
* confluent-kafka in turn is a wrapper around a C library called `librdkafka`_ (distributed as `librdkafka_dev`)
* librdkafka-dev does not currently have a compiled binary available for Linux/aarch64

As a result of the points above, if a package includes a dependency on confluent-kafka, developers will not be able to install
the package on certain machines or within certain Docker containers.
As a result of the points above, if a package includes a dependency on confluent-kafka, developers will not be able to install the package in Linux/aarch64 environments.

.. _confluent-kafka: https://github.com/confluentinc/confluent-kafka-python
.. _librdkafka: https://github.com/edenhill/librdkafka

Decision
--------

Instead of requiring confluent-kafka directly in base.in, we will wrap all imports in a `try...catch` block.
If the import fails, the library will log an informative message and attempt to fail gracefully.
Instead of requiring confluent-kafka directly in base.in, we will wrap all imports of `confluent_kafka`in a `try...catch` block. If the import fails, the library will log an informative message and any calls will fail gracefully.

Consequences
------------
This will make developers or other users of the edx-event-bus-kafka library responsible for installing
confluent-kafka in their own environment.
This will make developers or other users of the edx-event-bus-kafka library responsible for installing confluent-kafka in their own environments.

For edx.org, we will install confluent-kafka as part of creating the docker containers that will run the services
that use this library.

Rejected Alternatives
---------------------
* Make the entire `edx-event-bus-kafka` library an optional dependency in the services that use it (eg edx-platform, course-discovery)

This would require developers to install edx-event-bus-kafka when setting up their environment. This means
it would not be able to be updated with `make upgrade` in the same way we manage versions of all of our other packages.
Moreover, this would require separate commits to update the version of the package and update the code that uses it, meaning we
would have to use an expand-contract release model for every breaking change, which is highly error-prone.
This would require developers to install `edx-event-bus-kafka` separately when setting up their environment. This means it would not be able to be updated with `make upgrade` in the same way we manage versions of all of our other packages. Moreover, this would require separate commits to update the version of the package and update the code that uses it, meaning we would have to use an expand-contract release model for every breaking change. This goes against best practices, being highly error-prone.

We expect edx-event-bus-kafka to change more frequently than `confluent-kafka`, which is why we are more willing to adopt the optional dependency strategy for the latter.

* Keep both `confluent-kafka` and `edx-event-bus-kafka` as a required dependencies

While not necessarily causing problems for edx.org, this would break many community-hosted Open edX instances
While not necessarily causing problems for edx.org, this would break many community-hosted Open edX instances as well as many development environments.

0 comments on commit 8d5347e

Please sign in to comment.