From 4444d3fe590c4537a928c405070a574fbe4277ac Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Thu, 1 Sep 2022 09:13:48 -0400 Subject: [PATCH] refactor!: Move to new package structure to highlight public/internal distinction (#37) Document the public API in the README; add ADR to explain refactor. Also: - Fix some rST syntax in the changelog; fix a release heading - Add another missing `__init__.py` file (and fix lint exposed by that) - Small test improvement while I'm in there - Remove redundant consumer README (duplicated by main README) --- CHANGELOG.rst | 14 ++++-- README.rst | 3 ++ .../0006-public-api-and-app-organization.rst | 47 +++++++++++++++++++ edx_event_bus_kafka/__init__.py | 8 +++- edx_event_bus_kafka/consumer/README.rst | 9 ---- edx_event_bus_kafka/consumer/__init__.py | 3 -- edx_event_bus_kafka/internal/__init__.py | 7 +++ edx_event_bus_kafka/{ => internal}/config.py | 0 .../consumer.py} | 2 +- .../producer.py} | 2 +- .../{consumer => internal}/tests/__init__.py | 0 .../{ => internal}/tests/test_config.py | 11 ++++- .../tests/test_consumer.py} | 4 +- .../tests/test_producer.py} | 6 +-- .../management/commands/__init__.py | 5 ++ .../management/commands/consume_events.py | 4 +- .../management/commands/produce_event.py | 2 +- edx_event_bus_kafka/publishing/__init__.py | 0 18 files changed, 99 insertions(+), 28 deletions(-) create mode 100644 docs/decisions/0006-public-api-and-app-organization.rst delete mode 100644 edx_event_bus_kafka/consumer/README.rst delete mode 100644 edx_event_bus_kafka/consumer/__init__.py create mode 100644 edx_event_bus_kafka/internal/__init__.py rename edx_event_bus_kafka/{ => internal}/config.py (100%) rename edx_event_bus_kafka/{consumer/event_consumer.py => internal/consumer.py} (99%) rename edx_event_bus_kafka/{publishing/event_producer.py => internal/producer.py} (99%) rename edx_event_bus_kafka/{consumer => internal}/tests/__init__.py (100%) rename edx_event_bus_kafka/{ => internal}/tests/test_config.py (85%) rename edx_event_bus_kafka/{consumer/tests/test_event_consumer.py => internal/tests/test_consumer.py} (96%) rename edx_event_bus_kafka/{publishing/test_event_producer.py => internal/tests/test_producer.py} (96%) delete mode 100644 edx_event_bus_kafka/publishing/__init__.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7a9c7e2..a5da0c9 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,7 +16,15 @@ Unreleased * -[0.5.0] - 2022-08-31 +[0.6.0] - 2022-09-01 +******************** + +Changed +======= + +* **Breaking change**: Public API is now defined in ``edx_event_bus_kafka`` package and ``edx_event_bus_kafka.management.commands`` package; all other modules should be considered unstable and not for external use. + +[0.5.1] - 2022-08-31 ******************** Fixed @@ -32,8 +40,8 @@ Changed * **Breaking changes** in the producer module, refactored to expose a better API: - * Rather than `send_to_event_bus(...)`, relying code should now call `get_producer().send(...)`. - * The `sync` kwarg is gone; to flush and sync messages before shutdown, call `get_producer().prepare_for_shutdown()` instead. + * Rather than ``send_to_event_bus(...)``, relying code should now call ``get_producer().send(...)``. + * The ``sync`` kwarg is gone; to flush and sync messages before shutdown, call ``get_producer().prepare_for_shutdown()`` instead. * Clarify that config module is for internal use only. * Implementation changes: Only a single Producer is created, and is used for all signals. diff --git a/README.rst b/README.rst index 8f35819..13bcd52 100644 --- a/README.rst +++ b/README.rst @@ -33,6 +33,9 @@ For manual testing, see ``__. Documentation ************* +- Main API: ``edx_event_bus_kafka`` exposes ``get_producer`` and a Producer API class. See ``_ for how these will be documented and used in the future. +- Django management commands: ``edx_event_bus_kafka.management.commands.*`` expose ``Command`` classes + OEP-52 documentation: https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0052-arch-event-bus-architecture.html (TODO: `Set up documentation `_) diff --git a/docs/decisions/0006-public-api-and-app-organization.rst b/docs/decisions/0006-public-api-and-app-organization.rst new file mode 100644 index 0000000..d9d5007 --- /dev/null +++ b/docs/decisions/0006-public-api-and-app-organization.rst @@ -0,0 +1,47 @@ +6. Public API and App Organization +################################## + + +Status +****** + +Accepted + +Context +******* + +There has been no clear distinction between the public API in this package and the various internal, subject-to-change module attributes. This means we are more likely to end up with breaking changes, possibly without knowing it. Callers may also be unclear about which functions and classes they should be using. + +Decision +******** + +Put most of the code into packages with ``internal`` in their name, and import the public bits into modules that are not marked that way. + +- ``edx_event_bus_kafka/__init__.py`` imports various producer and consumer module attributes +- ``edx_event_bus_kafka/management/commands/*.py`` expose ``Command`` classes + +These will also be documented in the main README. + +The benefits of this setup include: + +* A clear designation of what is part of the public API. +* The ability to refactor the implementation without changing the API. +* A clear reminder to developers adding new code that it needs to be exposed if it is public. +* A clear reminder to developers using the library not to use code from the internal implementation. + +Consequences +************ + +Whenever a new class or function is added to the edx_django_utils public API, it should be implemented in the Django app's ``internal`` module and explicitly imported in its ``__init__.py`` module. + +There will be some additional friction when a developer looks up the docstrings of a public API element, since they'll first have to open the public module and then find the internal module. Having the public docstrings in a module marked "internal" also creates some dissonance. Our prediction is that this dissonance will be outweighed by the benefits of API stability. + +Rejected Alternatives +********************* + +None. + +References +********** + +This draws heavily on an ADR in ``edx-django-utils``: ``_ diff --git a/edx_event_bus_kafka/__init__.py b/edx_event_bus_kafka/__init__.py index 4209be6..cb1260e 100644 --- a/edx_event_bus_kafka/__init__.py +++ b/edx_event_bus_kafka/__init__.py @@ -1,5 +1,11 @@ """ Kafka implementation for Open edX event bus. + +Public API will be in this module for the most part. + +See ADR ``docs/decisions/0006-public-api-and-app-organization.rst`` for the reasoning. """ -__version__ = '0.5.1' +from edx_event_bus_kafka.internal.producer import EventProducerKafka, get_producer + +__version__ = '0.6.0' diff --git a/edx_event_bus_kafka/consumer/README.rst b/edx_event_bus_kafka/consumer/README.rst deleted file mode 100644 index 695639e..0000000 --- a/edx_event_bus_kafka/consumer/README.rst +++ /dev/null @@ -1,9 +0,0 @@ -Consumer -######## - -Purpose -******* - -This part of the library implements event bus consumer patterns using the Confluent Kafka API. - -During development, this app will be subject to frequent and rapid changes. Outside of testing this app, it is best to leave the ``EVENT_BUS_KAFKA_CONSUMERS_ENABLED`` setting off. diff --git a/edx_event_bus_kafka/consumer/__init__.py b/edx_event_bus_kafka/consumer/__init__.py deleted file mode 100644 index 66cf089..0000000 --- a/edx_event_bus_kafka/consumer/__init__.py +++ /dev/null @@ -1,3 +0,0 @@ -""" -Kafka consumer application. -""" diff --git a/edx_event_bus_kafka/internal/__init__.py b/edx_event_bus_kafka/internal/__init__.py new file mode 100644 index 0000000..53139cf --- /dev/null +++ b/edx_event_bus_kafka/internal/__init__.py @@ -0,0 +1,7 @@ +""" +Most of the implementation of the package is here and is internal-only. + +Public API will be in the ``edx_event_bus_kafka`` module for the most part. + +See ADR ``docs/decisions/0006-public-api-and-app-organization.rst`` for the reasoning. +""" diff --git a/edx_event_bus_kafka/config.py b/edx_event_bus_kafka/internal/config.py similarity index 100% rename from edx_event_bus_kafka/config.py rename to edx_event_bus_kafka/internal/config.py diff --git a/edx_event_bus_kafka/consumer/event_consumer.py b/edx_event_bus_kafka/internal/consumer.py similarity index 99% rename from edx_event_bus_kafka/consumer/event_consumer.py rename to edx_event_bus_kafka/internal/consumer.py index 695f21d..6c41152 100644 --- a/edx_event_bus_kafka/consumer/event_consumer.py +++ b/edx_event_bus_kafka/internal/consumer.py @@ -13,7 +13,7 @@ from openedx_events.learning.signals import SESSION_LOGIN_COMPLETED from openedx_events.tooling import OpenEdxPublicSignal -from edx_event_bus_kafka.config import get_schema_registry_client, load_common_settings +from .config import get_schema_registry_client, load_common_settings logger = logging.getLogger(__name__) diff --git a/edx_event_bus_kafka/publishing/event_producer.py b/edx_event_bus_kafka/internal/producer.py similarity index 99% rename from edx_event_bus_kafka/publishing/event_producer.py rename to edx_event_bus_kafka/internal/producer.py index 4503d10..ba8084c 100644 --- a/edx_event_bus_kafka/publishing/event_producer.py +++ b/edx_event_bus_kafka/internal/producer.py @@ -14,7 +14,7 @@ from openedx_events.event_bus.avro.serializer import AvroSignalSerializer from openedx_events.tooling import OpenEdxPublicSignal -from edx_event_bus_kafka.config import get_schema_registry_client, load_common_settings +from .config import get_schema_registry_client, load_common_settings logger = logging.getLogger(__name__) diff --git a/edx_event_bus_kafka/consumer/tests/__init__.py b/edx_event_bus_kafka/internal/tests/__init__.py similarity index 100% rename from edx_event_bus_kafka/consumer/tests/__init__.py rename to edx_event_bus_kafka/internal/tests/__init__.py diff --git a/edx_event_bus_kafka/tests/test_config.py b/edx_event_bus_kafka/internal/tests/test_config.py similarity index 85% rename from edx_event_bus_kafka/tests/test_config.py rename to edx_event_bus_kafka/internal/tests/test_config.py index c81b804..c12df29 100644 --- a/edx_event_bus_kafka/tests/test_config.py +++ b/edx_event_bus_kafka/internal/tests/test_config.py @@ -6,7 +6,7 @@ from django.test.utils import override_settings -from edx_event_bus_kafka import config +from edx_event_bus_kafka.internal import config # See https://github.com/openedx/event-bus-kafka/blob/main/docs/decisions/0005-optional-import-of-confluent-kafka.rst try: @@ -16,6 +16,9 @@ class TestSchemaRegistryClient(TestCase): + """ + Test client creation. + """ def test_unconfigured(self): assert config.get_schema_registry_client() is None @@ -23,8 +26,14 @@ def test_configured(self): with override_settings(EVENT_BUS_KAFKA_SCHEMA_REGISTRY_URL='http://localhost:12345'): assert isinstance(config.get_schema_registry_client(), SchemaRegistryClient) + # Check if it's cached, too + assert config.get_schema_registry_client() is config.get_schema_registry_client() + class TestCommonSettings(TestCase): + """ + Test loading of settings common to producer and consumer. + """ def test_unconfigured(self): assert config.load_common_settings() is None diff --git a/edx_event_bus_kafka/consumer/tests/test_event_consumer.py b/edx_event_bus_kafka/internal/tests/test_consumer.py similarity index 96% rename from edx_event_bus_kafka/consumer/tests/test_event_consumer.py rename to edx_event_bus_kafka/internal/tests/test_consumer.py index 50331e7..598a310 100644 --- a/edx_event_bus_kafka/consumer/tests/test_event_consumer.py +++ b/edx_event_bus_kafka/internal/tests/test_consumer.py @@ -11,7 +11,7 @@ from openedx_events.learning.data import UserData, UserPersonalData from openedx_events.tooling import OpenEdxPublicSignal -from edx_event_bus_kafka.consumer.event_consumer import KafkaEventConsumer +from edx_event_bus_kafka.internal.consumer import KafkaEventConsumer from edx_event_bus_kafka.management.commands.consume_events import Command @@ -126,7 +126,7 @@ class TestCommand(TestCase): """ @override_settings(EVENT_BUS_KAFKA_CONSUMERS_ENABLED=False) - @patch('edx_event_bus_kafka.consumer.event_consumer.KafkaEventConsumer._create_consumer') + @patch('edx_event_bus_kafka.internal.consumer.KafkaEventConsumer._create_consumer') def test_kafka_consumers_disabled(self, mock_create_consumer): call_command(Command(), topic='test', group_id='test', signal='') assert not mock_create_consumer.called diff --git a/edx_event_bus_kafka/publishing/test_event_producer.py b/edx_event_bus_kafka/internal/tests/test_producer.py similarity index 96% rename from edx_event_bus_kafka/publishing/test_event_producer.py rename to edx_event_bus_kafka/internal/tests/test_producer.py index 3ebb1e6..1bce56e 100644 --- a/edx_event_bus_kafka/publishing/test_event_producer.py +++ b/edx_event_bus_kafka/internal/tests/test_producer.py @@ -12,7 +12,7 @@ from openedx_events.event_bus.avro.serializer import AvroSignalSerializer from openedx_events.learning.data import UserData, UserPersonalData -import edx_event_bus_kafka.publishing.event_producer as ep +import edx_event_bus_kafka.internal.producer as ep # See https://github.com/openedx/event-bus-kafka/blob/main/docs/decisions/0005-optional-import-of-confluent-kafka.rst try: @@ -94,7 +94,7 @@ def test_get_producer_configured(self): ): assert isinstance(ep.get_producer(), ep.EventProducerKafka) - @patch('edx_event_bus_kafka.publishing.event_producer.logger') + @patch('edx_event_bus_kafka.internal.producer.logger') def test_on_event_deliver(self, mock_logger): fake_event = MagicMock() fake_event.topic.return_value = 'some_topic' @@ -112,7 +112,7 @@ def test_on_event_deliver(self, mock_logger): # Mock out the serializers for this one so we don't have to deal # with expected Avro bytes -- and they can't call their schema server. @patch( - 'edx_event_bus_kafka.publishing.event_producer.get_serializers', autospec=True, + 'edx_event_bus_kafka.internal.producer.get_serializers', autospec=True, return_value=( lambda _key, _ctx: b'key-bytes-here', lambda _value, _ctx: b'value-bytes-here', diff --git a/edx_event_bus_kafka/management/commands/__init__.py b/edx_event_bus_kafka/management/commands/__init__.py index e69de29..3abcb6c 100644 --- a/edx_event_bus_kafka/management/commands/__init__.py +++ b/edx_event_bus_kafka/management/commands/__init__.py @@ -0,0 +1,5 @@ +""" +Management commands for the Kafka implementation. + +(Django requires this package structure.) +""" diff --git a/edx_event_bus_kafka/management/commands/consume_events.py b/edx_event_bus_kafka/management/commands/consume_events.py index 6a5bdda..f0ffdba 100644 --- a/edx_event_bus_kafka/management/commands/consume_events.py +++ b/edx_event_bus_kafka/management/commands/consume_events.py @@ -1,7 +1,5 @@ """ Makes ``consume_events`` management command available. - -Implements required ``APP.management.commands.*.Command`` structure. """ -from edx_event_bus_kafka.consumer.event_consumer import ConsumeEventsCommand as Command # pylint: disable=unused-import +from edx_event_bus_kafka.internal.consumer import ConsumeEventsCommand as Command # pylint: disable=unused-import diff --git a/edx_event_bus_kafka/management/commands/produce_event.py b/edx_event_bus_kafka/management/commands/produce_event.py index 4eb22df..977014a 100644 --- a/edx_event_bus_kafka/management/commands/produce_event.py +++ b/edx_event_bus_kafka/management/commands/produce_event.py @@ -10,7 +10,7 @@ from django.core.management.base import BaseCommand from django.utils.module_loading import import_string -from edx_event_bus_kafka.publishing.event_producer import get_producer +from edx_event_bus_kafka.internal.producer import get_producer logger = logging.getLogger(__name__) diff --git a/edx_event_bus_kafka/publishing/__init__.py b/edx_event_bus_kafka/publishing/__init__.py deleted file mode 100644 index e69de29..0000000