Use thimble

[lvh]

WIP, with some review points so it doesn't get too off-track.

[lvh]

Some points that I hope @manishtomar can answer :)

• This changes Lock and SetPartitioner to no longer necessarily be externally visible classes. (I may still have to change that, but I don't think they're still necessary). ISTR that the only way you're supposed to use this is client.Lock and client.SetPartitioner, so I don't think it'll matter, but...
• The docstring for add_listener claimed that this should only be called from the reactor thread. Is that correct, and, if so, why?

[manishtomar]

This changes Lock and SetPartitioner to no longer necessarily be externally visible classes. (I may still have to change that, but I don't think they're still necessary)

I was mainly trying to replicate same design of KazooClient.

ISTR that the only way you're supposed to use this is client.Lock and client.SetPartitioner

Not really. The other way (as per kazoo lib) of using is to create the objects manually with KazooClient instance as argument. These methods are just helpers to that original method.

The docstring for add_listener claimed that this should only be called from the reactor thread. Is that correct, and, if so, why?

Since it has mapping of listeners that can get corrupted when called from multiple threads. In general txkazoo apis are expected to be used in reactor thread only. This is why it has internal mapping of listeners to replay callback in reactor thread.

[lvh]

I was mainly trying to replicate same design of KazooClient.

Sure, that makes sense :)

Not really. The other way (as per kazoo lib) of using is to create the objects manually with KazooClient instance as argument. These methods are just helpers to that original method.

Right; but I thought you said at some point that the only way we actually intend to use it is through a client? I could just keep the classes, but since their signature has to change (previously it didn't take a thread pool), it doesn't do much good.

Since it has mapping of listeners that can get corrupted when called from multiple threads. In general txkazoo apis are expected to be used in reactor thread only. This is why it has internal mapping of listeners to replay callback in reactor thread.

Hm; I thought STORE_SUBSCR was guaranteed to be threadsafe in Python?

Thanks for all the comments :)

[lvh]

Re: Lock and SetPartitioner being externally visible classes, to clarify: I understand why you did that, I'm asking if it's okay if they no longer are :) (Even if they are, they'd probably get turned into functions with a different signature.)

[manishtomar]

I understand why you did that, I'm asking if it's okay if they no longer are :)

I think its fine if they are not there. Especially since they'll also have the take reactor and thread pool. And thanks a lot for working on this :)

[lvh]

It's a bit unfortunate that SetPartitioner does a bunch of blocking IO in __init__, because it means we can never get rid of that class :)

[coveralls]

Coverage increased (+3.9%) when pulling d626790 on lvh:use-thimble into 61fabad on rackerlabs:master.

[lvh]

I think this is ready for review.

[manishtomar]

This changes the interface. That should be fine right?

[lvh]

Yeah. Since the old interface didn't take an explicit pool (and instead just messed with the reactor pool), if we use the reactor pool too, we can't see if the txkazoo issues we're seeing right now are somehow related to using the reactor pool. (I don't think that's a credible cause, but it's one of the few this experiment may tell us about.)

[manishtomar]

Docstrings will be nice :)

[lvh]

Done: 850ecd1

[manishtomar]

This is great! It will be nice to have some tests for SetPartitioner. Mainly check if __iter__ is delegated to internal object, if its methods come from thimble and if it is created in thread pool with correct args.

[lvh]

Thanks for the review @manishtomar ! Pushed the commits addressing the review comments :)

[lvh]

Oops, missed that thing about iteration. Fixed that too. I explicitly tested behavior (i.e. that you actually get the underlying __iter__ results) instead of inheritance, so the FakeSetPartitioner grew an extra __iter__ method :)

[coveralls]

Coverage increased (+4.69%) when pulling e5041b4 on lvh:use-thimble into 61fabad on rackerlabs:master.

[manishtomar]

LGTM!

[coveralls]

Coverage increased (+4.69%) when pulling d0b5f3c on lvh:use-thimble into 61fabad on rackerlabs:master.