-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
a14c85b
to
ee0fd50
Compare
ee0fd50
to
8048e1f
Compare
8048e1f
to
de3f27f
Compare
django_kafka/retry/topic.py
Outdated
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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}"
There was a problem hiding this comment.
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.
de3f27f
to
88b70d9
Compare
88b70d9
to
50aadec
Compare
@bodja please review again, thanks! |
django_kafka/retry/topic.py
Outdated
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 |
There was a problem hiding this comment.
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
50aadec
to
8a8bf2e
Compare
f0ef2ec
to
d24d30c
Compare
d24d30c
to
61c9b35
Compare
No description provided.