Readmefix1 #355
Readmefix1 #355
Conversation
README.md
Outdated
|
|
||
| for data in some_data_source: | ||
| p.produce('mytopic', data.encode('utf-8')) | ||
| # Trigger delivery report callbacks from previous produce() calls |
There was a problem hiding this comment.
maybe 'trigger any available delivery report callbacks from previous produce calls'.
README.md
Outdated
| # Trigger delivery report callbacks from previous produce() calls | ||
| p.poll(0) | ||
|
|
||
| # Asynchronously produce a message, the optional but recommended |
There was a problem hiding this comment.
i'd remove 'but recommended'.
Also, I think you should 1. explicitly say under exactly what conditions poll is required and 2. explicitly say that if it's not called, messages will pile up indefinitely. My understanding is that errors are also exposed via the same queue, so poll should be called regardless of whether delivery reports are enabled. Is that right?
There was a problem hiding this comment.
If you don't register an error callback there will be no error events so you don't need to call poll() for that purpose.
We unfortunately can't do the same with delivery reports since they can be set on a per-produce() call basis.
But I don't want to flesh out this example too much, I think I'll go with using a delivery report callback and not speaking of that it is optional. People should be interested in knowing their messages are actually delivered.
There was a problem hiding this comment.
yep, the pr's a step forward so i'm fine to merge. a point here is that by exposing poll, all of these details are also effectively exposed, whereas if the poll call is managed by the producer, they are effectively encapsulated inside the producer - another reason why not having a manual poll api (if it were possible) would reduce complexity considerably.
|
Aside from clarifying the comments around poll, LGTM. Happy to re-review them. Didn't spot anything wrong with the header dict support implementation, and you have tests. |
No description provided.