Skip to content

Added error callback for propagating generic errors from librdkafka to app #15

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

Merged
merged 5 commits into from
Aug 4, 2016

Conversation

edenhill
Copy link
Contributor

@edenhill edenhill commented Jul 6, 2016

No description provided.

@@ -78,6 +78,9 @@ The Python bindings also provide some additional configuration properties:
* ``default.topic.config``: value is a dict of topic-level configuration
properties that are applied to all used topics for the instance.

* ``error_cb``: Callback for generic/global error events. This callback is served by
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be including the argument lists for these callbacks?

@ewencp
Copy link
Contributor

ewencp commented Jul 8, 2016

@edenhill LGTM, but it has very limited testing, just that the setup works. For example, we don't actually evaluate that the expected set of arguments matches. Anything we can do after creating the producer/consumer in the unit tests to evaluate that invoking the callback actually works?

Also, should we start tracking changelogs like we do for other projects (e.g. http://docs.confluent.io/3.0.0/schema-registry/docs/changelog.html). We normally update these just before release since its easier than updating with each patch, but given that clients need to release independently and we're doing a new tag + release for this asap....

@edenhill
Copy link
Contributor Author

Updated with unit test and callback arg documentation.

if (self->u.Consumer.on_assign)
Py_VISIT(self->u.Consumer.on_assign);
if (self->u.Consumer.on_revoke)
Py_VISIT(self->u.Consumer.on_revoke);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing visiting on_commit?

@ewencp
Copy link
Contributor

ewencp commented Jul 13, 2016

@edenhill One last question, but LGTM otherwise.

@paulcavallaro
Copy link

Wanted to ping on this pull request because isssue #14 is blocking our adoption of this library. Thanks,

-Paul

@ewencp
Copy link
Contributor

ewencp commented Aug 4, 2016

@paulcavallaro Should see some follow up soon, sorry for the delay. My last comment was right before @edenhill went on vacation, but he is returning imminently!

@ewencp
Copy link
Contributor

ewencp commented Aug 4, 2016

@edenhill I'm seeing this:

$ examples/integration_test.py localhost:9092
Using confluent_kafka module version 0.9.1 (0x90100)
Using librdkafka version 0.9.1 (0x901ff)
('==============================', 'Verifying Producer', '==============================')
Traceback (most recent call last):
  File "examples/integration_test.py", line 366, in <module>
    verify_producer()
  File "examples/integration_test.py", line 86, in verify_producer
    p = confluent_kafka.Producer(**conf)
cimpl.KafkaException: KafkaError{code=_INVALID_ARG,val=-186,str="Property "error_cb" must be set through dedicated .._set_..() function"}

@ewencp
Copy link
Contributor

ewencp commented Aug 4, 2016

@edenhill Ok, nm, must have had something stale somewhere. Reran in a virtualenv and its fine.

@edenhill
Copy link
Contributor Author

edenhill commented Aug 4, 2016

That looks like you used an old confluent_kafka python module version without error_cb support (thus the fallthru to librdkafka)

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