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

Dynamically define dlq and retry topics (GSI-1175) #143

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

TheByronHimes
Copy link
Member

@TheByronHimes TheByronHimes commented Nov 21, 2024

akafka/config.py

  • Removed the config values for the two topics (kafka_dlq_topic and kafka_retry_topic). The former is generated by appending KafkaConfig.service_name and -dlq to the end of the vanilla topic name, with a . between. Example: regular topic is 'some-topic', the service name is 'my-service', and the dlq topic that 'my-service' uses for erroring events from 'some-topic' is 'some-topic.my-service-dlq'.
  • Removed the validator because the fields it validated are gone

akafka/eventsub.py

  • Add a small function for getting the service name from a DLQ topic name
  • Dynamically form and subscribe to retry topic based on configured service_name.
  • Remove original_topic header for the first half of the DLQ flow (it's not needed or included when publishing a failed event to the DLQ because the topic name is preserved in the DLQ topic's name)
  • In KafkaEventSubscriber._extract_info(), it will get the original topic value from the DLQ topic name instead of the old header
  • In KafkaDLQSubscriber, The retry topic is no longer an init variable because that information is, again, pulled from the service name (which is embedded in the DLQ topic name).

Tests

  • Since the config is simplified, we don't need a bunch of tests to make sure the config validation works.
  • Renamed the subscriber instances to either event_subscriber or dlq_subscriber for clarity
  • Moved "test-topic" and "test-type" into constants
  • Added a few comments for clarity
  • Updated the log assertions that included topic names (previously the test names were dlq or retry, but these are now test-topic.test-dlq or test-retry)
  • Replaced one use of a translator with an AsyncMock because the translator wasn't actually used (around L590)
  • Use constant DLQ and retry topics since the tests all use the same topic and event types

Remove accidental subscription to dlq topics in main class (tests passed earlier because the names had '-' instead of '.', meaning the topics were not touched in test)
@coveralls
Copy link

coveralls commented Nov 21, 2024

Pull Request Test Coverage Report for Build 11972958588

Details

  • 15 of 18 (83.33%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 92.311%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/hexkit/providers/akafka/provider/eventsub.py 14 17 82.35%
Totals Coverage Status
Change from base Build 11913210287: -0.1%
Covered Lines: 2053
Relevant Lines: 2224

💛 - Coveralls

@TheByronHimes TheByronHimes requested a review from Cito November 22, 2024 07:45
@TheByronHimes TheByronHimes marked this pull request as ready for review November 22, 2024 07:45
Cito
Cito previously approved these changes Nov 22, 2024
src/hexkit/providers/akafka/provider/eventsub.py Outdated Show resolved Hide resolved
tests/unit/test_dlqsub.py Outdated Show resolved Hide resolved
@TheByronHimes TheByronHimes merged commit da9f023 into main Nov 22, 2024
8 checks passed
@TheByronHimes TheByronHimes deleted the feature/automate_dlq_topic_naming_GSI-1175 branch November 22, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants