Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adds add_callback/add_errback example to docs #1441

Merged
merged 2 commits into from
Mar 29, 2018
Merged

Conversation

Berkodev
Copy link
Contributor

After spending quite some time figuring out how to properly add callbacks/errbacks to the producer.send future obj, decided to add a basic example to the docs (Issue #1256 ).

I didn't really know where to put it as there seemed to be no examples in the docs outside of "Usage Overview", so I just put it there.

I am willing to expand upon this if needed, but I'll need to know where exactly to put it in and some guidance as to what you want to see in terms of explanation/example.

Have a good day!

@dpkp
Copy link
Owner

dpkp commented Mar 13, 2018 via email

@Berkodev
Copy link
Contributor Author

No worries, I'm actually on a vacation myself next week, so will be available the following week if needed.

Enjoy the rest of your vacation!

docs/usage.rst Outdated
print(record_metadata.offset)

def on_send_error(excp):
log.exception()
Copy link
Contributor

Choose a reason for hiding this comment

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

Just doublechecking, since I'm not sure how this works:
Is the send exception always going to be the most recent one raised? If you screwed up your callback somehow and threw an exception, it would hide the send exception.

Perhaps it'd be better to illustrate how to pass the excp into log.error()? This is honestly less of a kafka-python thing, and more just showing clearly how python logging works, as we often get questions on the issue tracker about that as well...

Just a thought, I do not have a strong opinion, and I am not even 100% sure how this works in this particular example as I haven't played with it...

Copy link
Owner

Choose a reason for hiding this comment

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

I agree -- log.exception() may not always work here because we don't guarantee that the errback is evaluated within the context of a exception handler. It would be better to pass the exception explicitly:

def on_send_error(excp):
    log.error('I am an errback', exc_info=excp)

Copy link
Owner

@dpkp dpkp left a comment

Choose a reason for hiding this comment

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

Requesting changes re: log.exception / log.error

Copy link
Contributor Author

@Berkodev Berkodev left a comment

Choose a reason for hiding this comment

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

Makes sense, I was trying to use the same expressions used in the file (line 71) for the sake of standard, but I can see how that would be an issue in an async context.

@dpkp dpkp merged commit 4267ed5 into dpkp:master Mar 29, 2018
@dpkp
Copy link
Owner

dpkp commented Mar 29, 2018

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants