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: add retry and dead letter topic behaviour, refs #8 #9

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

stefan-cardnell-rh
Copy link
Contributor

No description provided.

@stefan-cardnell-rh stefan-cardnell-rh changed the title feat: add retry and dead letter topic behaviour, refs #8 Draft: feat: add retry and dead letter topic behaviour, refs #8 Sep 6, 2024
@stefan-cardnell-rh stefan-cardnell-rh changed the title Draft: feat: add retry and dead letter topic behaviour, refs #8 feat: add retry and dead letter topic behaviour, refs #8 Sep 6, 2024
match = re.search(r"\.retry\.([0-9]+)$", topic_name)
return int(match.group(1)) if match else 0

def get_produce_name(self, topic_name: str, attempt: int) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the first attempt need a special handling?

Copy link
Contributor

Choose a reason for hiding this comment

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

we could always return return f"{self.group_id}.{topic_name}.retry.{attempt}"

Copy link
Contributor Author

@stefan-cardnell-rh stefan-cardnell-rh Sep 10, 2024

Choose a reason for hiding this comment

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

Depending on the attempt, msg.topic() can be different. For attempt 1, the msg.topic() is the original topic. For greater, msg.topic() is the retry topic ({self.group_id}.{topic_name}.retry.{attempt}). So depending on the attempt, I have to append retry.1, or regex sub the attempt numbers.

I have access to the main topic, but I can't do:

return {self.group_id}.{self.main_topic.name}.retry.{attempt}

Because self.main_topic.name may be regex hitting multiple topics, whereas I need a concrete topic name, not a regex, which means I need to get it from msg.topic().

I think maybe we should rename the name property because it gives the impression it's a concrete name and not possibly a regex. In Confluent Kafka it's just called topic. But if I put a topic property in the Topic class, we'd be accessing topic.topic a lot.

django_kafka/registry.py Show resolved Hide resolved
django_kafka/retry/__init__.py Outdated Show resolved Hide resolved
django_kafka/retry/consumer.py Outdated Show resolved Hide resolved
django_kafka/retry/topic.py Outdated Show resolved Hide resolved
@stefan-cardnell-rh
Copy link
Contributor Author

@bodja please review again, thanks!

msg: the message that failed, this may come from the main topic or retry topic
exc: Exception that caused the failure
"""
settings = self.main_topic.retry_settings
Copy link
Contributor

Choose a reason for hiding this comment

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

lets rename this variable to smth else, to avoid conflicts when importing conf.settings

@stefan-cardnell-rh stefan-cardnell-rh force-pushed the 8-retry-dead-letter-queue branch 2 times, most recently from f0ef2ec to d24d30c Compare September 12, 2024 14:49
@stefan-cardnell-rh stefan-cardnell-rh merged commit a050244 into main Sep 12, 2024
12 checks passed
@stefan-cardnell-rh stefan-cardnell-rh deleted the 8-retry-dead-letter-queue branch September 12, 2024 15:24
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.

2 participants