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

Block & fail early if produce requests don't work #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

grdryn
Copy link

@grdryn grdryn commented Oct 29, 2021

The kafka-python docs[1] note that producer.flush() only blocks
until all messages are put on the network, and it doesn't guarantee
delivery or success. Even if the provided credentials correspond to a
principal that doesn't have permission to produce to the topic,
everything might look like it has succeeded to the user:

>>> produce_messages(1, 10, 2)
sending {"txt": "hello 1"}
sending {"txt": "hello 2"}
sending {"txt": "hello 3"}
sending {"txt": "hello 4"}
sending {"txt": "hello 5"}
sending {"txt": "hello 6"}
sending {"txt": "hello 7"}
sending {"txt": "hello 8"}
sending {"txt": "hello 9"}
sending {"txt": "hello 10"}

The change proposed here (also based on the docs[1]) will cause each
iteration of the loop to block until the produce request has either
succeeded or failed:

>>> produce_messages(1, 10, 2)
sending {"txt": "hello 1"}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 25, in produce_messages
  File "/var/home/gryan/.local/lib/python3.10/site-packages/kafka/producer/future.py", line 65, in get
    raise self.exception # pylint: disable-msg=raising-bad-type
kafka.errors.TopicAuthorizationFailedError: [Error 29] TopicAuthorizationFailedError: python-test

NOTE: I haven't actually tried this notebook out, as I don't know how
to run notebooks yet and haven't set up an environment for that -- I
was just trying to understand why a buddy was not seeing a failure
when they should have, when running the code from this notebook. I
just copied the code from here into my local python interpreter. I
don't know if there's any special way that exceptions should be
handled in notebooks or anything like that.

[1] https://pypi.org/project/kafka-python/

The kafka-python docs[1] note that `producer.flush()` only blocks
until all messages are put on the network, and it doesn't guarantee
delivery or success. Even if the provided credentials correspond to a
principal that doesn't have permission to produce to the topic,
everything might look like it has succeeded to the user:

```
>>> produce_messages(1, 10, 2)
sending {"txt": "hello 1"}
sending {"txt": "hello 2"}
sending {"txt": "hello 3"}
sending {"txt": "hello 4"}
sending {"txt": "hello 5"}
sending {"txt": "hello 6"}
sending {"txt": "hello 7"}
sending {"txt": "hello 8"}
sending {"txt": "hello 9"}
sending {"txt": "hello 10"}
```

The change proposed here (also based on the docs[1]) will cause each
iteration of the loop to block until the produce request has either
succeeded or failed:

```
>>> produce_messages(1, 10, 2)
sending {"txt": "hello 1"}
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 25, in produce_messages
  File "/var/home/gryan/.local/lib/python3.10/site-packages/kafka/producer/future.py", line 65, in get
    raise self.exception # pylint: disable-msg=raising-bad-type
kafka.errors.TopicAuthorizationFailedError: [Error 29] TopicAuthorizationFailedError: python-test
```

NOTE: I haven't actually tried this notebook out, as I don't know how
to run notebooks yet and haven't set up an environment for that -- I
was just trying to understand why a buddy was not seeing a failure
when they should have, when running the code from this notebook. I
just copied the code from here into my local python interpreter. I
don't know if there's any special way that exceptions should be
handled in notebooks or anything like that.

[1] https://pypi.org/project/kafka-python/
@grdryn
Copy link
Author

grdryn commented Oct 29, 2021

@cfchase Hi! 👋 You seem to be the owner of this project, so just pinging you about this in case you don't have notifications turned on for all activity here. 🐻

@k-wall
Copy link

k-wall commented Oct 29, 2021

@grdryn thanks for taking the initiative and offering the fix. (which lgtm btw)

@cfchase
Copy link
Collaborator

cfchase commented Nov 10, 2021

@grdryn thanks for submitting this! Sorry I just noticed. I'll test it out and get it merged.

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