Skip to content
This repository was archived by the owner on Mar 24, 2021. It is now read-only.

Conversation

yungchin
Copy link
Contributor

This closes #238.

Error responses from kafka previously only appeared in log output. With the changes here, produce() returns a concurrent.futures.Future, through which any errors that persist after max_retries can be communicated to user code.

For sync=True, we simply evaluate the Future immediately, so that kafka error responses are raised directly from produce().

This touches a few more lines than a minimal implementation would have done, I hope that's ok. I felt that slapping yet another item (ie the delivery_future) onto message_partition_tup was just going to get too unwieldly, so instead of message_partition_tup we're now moving instances of Message around. The second structural change is in _send_request(), where to_retry previously was an iterable of messages, but given that we were now going to add exceptions to that, and the error responses always apply to a whole MessageSet, to_retry is now an iterable of MessageSet, Exception.

The tuples handed to _produce() were starting to look like full-on
Messages anyway.  In preparation for work that would add yet another
element to the tuple (a delivery-reporting future), I figured we might
as well "upgrade" to a real Message.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
I've so far only done some clumsy testing with this, where I'd manually
disconnect broker connections, and wait for SocketDisconnectedError to
emerge from the Future - that works.  Test suite expansion coming soon.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
…utures

This pulls in a fix to handle required_acks=0, which will break this
branch (because that setting would leave futures pending forever).

* parsely/master: (22 commits)
  increment dev version
  raise helpful error if kafka <0.8.2 is in use during offset manager discovery
  include kafka version at readme top
  cluster: fix bug with internal topic lookups
  test_cluster: demonstrate issue #277
  producer: don't expect response if required_acks=0
  test_producer: add test demonstrating issue #278
  allow wheels
  fix rst
  update comparison link
  increment version
  keep one strong reference to the cluster
  lower log level for common situation
  remove unused import
  hold a weak reference to topic in partition
  use typeerror, not valueerror
  better changelog
  enfore breaking api change with a runtime error
  cluster: improve topic auto-creation
  cluster: fix topic auto-create (even on fresh cluster)
  ...

Conflicts:
	pykafka/producer.py
See also the related fix on the (future-less) master branch, c434696,
and issue #278 for context.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
This adds a test for the new producer futures feature, and immediately
exposed a bug: I pushed an exception type onto the future instead of an
instance.  We fix that here.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@yungchin
Copy link
Contributor Author

yungchin commented Oct 2, 2015

@rduplain @emmett9001 @kbourgoin I've just updated the description above. Would be grateful if you can check that this makes sense in terms of interface - and implementation of course.

@yungchin yungchin removed their assignment Oct 2, 2015
@kbourgoin
Copy link
Contributor

Related to #238.

@kbourgoin
Copy link
Contributor

Connected to #238.

@yungchin yungchin mentioned this pull request Oct 10, 2015
@yungchin yungchin mentioned this pull request Oct 23, 2015
11 tasks
@yungchin
Copy link
Contributor Author

@emmett9001 can I bug you to review yet another one? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

[nitpick] No need for this linebreak, I think. I've been using 90 characters as a length limit, since github windows hold at least that without scrolling left to right.

@emmettbutler emmettbutler added this to the 2.1.0 milestone Oct 28, 2015
@yungchin
Copy link
Contributor Author

Thanks, yep, that's it! There are also some convenience functions in the futures module, with which in your example you could split the list up in completed and pending futures, and throw the pending futures back on the list for the next round (otherwise checking future.exception() will block until the future is completed). I should probably steal that example and stick it somewhere in the docs, actually.

@emmettbutler
Copy link
Contributor

Yes, I think it's a good idea to include a code sample like this right above your paragraph in the README on how to use the futures.

As per discussion on #269.  In order for this to work, parent classes
also needed to be given `__slots__` (otherwise instances would still
have a `__dict__`).

A few tests used the instance's `__dict__` directly and thus needed a
tweak.  While working that out, I found that `test_snappy_decompression`
wasn't running, because it had been misnamed.  The test wasn't even
py3-compatible yet, but luckily the code it tests was.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@yungchin yungchin force-pushed the feature/producer-futures branch from 9b7ba1d to 3c38c82 Compare October 29, 2015 00:38
This exercises the path in the new producer-futures code where the error
encountered is recoverable.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@yungchin yungchin force-pushed the feature/producer-futures branch from 3c38c82 to 92d1291 Compare October 29, 2015 00:57
Just some cleanup. Should have done this as part of defining
Message.__slots__

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Users will likely always want to have the message content stapled
together with the delivery-future, for context when delivery fails, so
doing that for them makes things that bit more convenient.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@yungchin yungchin force-pushed the feature/producer-futures branch from 5fb609c to 7aa90e6 Compare October 29, 2015 23:12
@yungchin
Copy link
Contributor Author

Ok, I was poking around a bit with sys.getsizeof and the __slots__ change seems to reduce Message overhead by 10x (Message.__dict__ was over 1kB on py2 at least). However.... Future and its members weigh in at about twice that. The BDFL giveth, and the BDFL taketh away, or something like that.

@emmettbutler
Copy link
Contributor

Oh wow. In that case we may want a way to turn off futures with a kwarg or something. We should keep that in mind if memory usage ever becomes a problem, but I don't think we necessarily need to add it now.

@yungchin
Copy link
Contributor Author

With the changes I made as part of f4f6df1, if the user ignores the future, it's gone immediately (the message only holds a weakref to it). So you'd still have the overhead of creating and destroying one, but you wouldn't have one lying around for every message in your outbound-queue.

(The weakref is luckily not in possession of a __dict__, so it's a lot smaller)

emmettbutler added a commit that referenced this pull request Oct 30, 2015
@emmettbutler emmettbutler merged commit 52ae7a1 into master Oct 30, 2015
@emmettbutler emmettbutler deleted the feature/producer-futures branch October 30, 2015 22:09
@emmettbutler emmettbutler restored the feature/producer-futures branch November 10, 2015 23:09
yungchin added a commit that referenced this pull request Nov 11, 2015
As per discussion on #269.  In order for this to work, parent classes
also needed to be given `__slots__` (otherwise instances would still
have a `__dict__`).

A few tests used the instance's `__dict__` directly and thus needed a
tweak.  While working that out, I found that `test_snappy_decompression`
wasn't running, because it had been misnamed.  The test wasn't even
py3-compatible yet, but luckily the code it tests was.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
(cherry picked from commit 6b4b1ce)
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
yungchin added a commit that referenced this pull request Nov 18, 2015
As per discussion on #269.  In order for this to work, parent classes
also needed to be given `__slots__` (otherwise instances would still
have a `__dict__`).

A few tests used the instance's `__dict__` directly and thus needed a
tweak.  While working that out, I found that `test_snappy_decompression`
wasn't running, because it had been misnamed.  The test wasn't even
py3-compatible yet, but luckily the code it tests was.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
(cherry picked from commit 6b4b1ce)
Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@dan-blanchard dan-blanchard deleted the feature/producer-futures branch December 10, 2016 03:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return futures from Producer.produce()
4 participants