-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix: Correctly raise producer exceptions #660
Conversation
Deploying with Cloudflare Pages
|
2d5ab72
to
18ee1b9
Compare
karapace/backup/api.py
Outdated
|
||
|
||
def _handle_producer_send( | ||
instruction: ProducerSend, | ||
producer: KafkaProducer, | ||
) -> None: | ||
check_producer_exception() |
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.
issue: Let's document (with comment) the reason why we're also doing this here. Is it to reach a stop quicker than waiting for the call in restore_backup()
?
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.
Yes, this call is made here to avoid calling send()
for all messages if there was an error earlier.
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.
This still applies with the new change-set. For a reader of the code, it won't be obvious why the check method is called in two different places. I think it's important to document, with a comment in the code, why we make the post-send call: it's an optimization. And that one of the calls is fundamental for correct behavior, and one is not.
18ee1b9
to
f2e65a0
Compare
f2e65a0
to
eb19aa3
Compare
434ea31
to
c51e3a4
Compare
karapace/backup/api.py
Outdated
_producer_exception = None | ||
|
||
def _producer_error_callback(exception: Exception) -> None: | ||
LOG.error("Producer error: %r", exception) |
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.
LOG.error("Producer error: %r", exception) | |
LOG.error("Producer error callback", exc_info=exception) |
17d53ed
to
5f7e5e3
Compare
5f7e5e3
to
cd43ef6
Compare
Fix a bug in which producer exceptions were raised inside the I/O thread instead of the main one.
cd43ef6
to
86fac38
Compare
Fix a bug in which producer exceptions were raised inside the I/O thread instead of the main one.