[python zookeeper] Add member_id to ServiceInstance object if provided.

[dogbunny]

Add member_id to ServiceInstance object if supplied when creating it and make that the default action when creating a serverset list. This is mostly useful for reconciling issues with the serverset e.g. duplicate registration.

Tests all pass: $ ./pants test tests/python/twitter/common/zookeeper:all 11:22:40 00:02 [chroot]============== test session starts =============== platform darwin -- Python 2.7.10 -- py-1.4.31 -- pytest-2.6.4 plugins: cov, timeout collected 85 items

                 tests/python/twitter/common/zookeeper/kazoo_client_test.py .
                 tests/python/twitter/common/zookeeper/group/test_active_kazoo_group.py ................................................
                 tests/python/twitter/common/zookeeper/group/test_kazoo_group.py ..................
                 tests/python/twitter/common/zookeeper/serverset/test_endpoint.py ............
                 tests/python/twitter/common/zookeeper/serverset/test_kazoo_serverset.py .....
                 tests/python/twitter/common/zookeeper/serverset/test_serverset_unit.py .

                 =========== 85 passed in 23.80 seconds ===========

[Yasumoto]

Also saw the review from @atollena, thanks for the docstring on the method!

Based on the addition in tests/python/twitter/common/zookeeper/serverset/test_serverset_unit.py, this member_id field is expected to be added by whatever library/announcer is publishing to ZK?

[dogbunny]

No, it's actually part of the Member object and should be assigned by the ZK server. In our case we just weren't saving the information anywhere it could be accessed later either in the ServiceInstance object or the ServerSet itself. You only get the individual ServiceInstances (the twitter interpretation of a zk member node) when you iterate over the ServerSet object at which point you loose the member_id data so rather than changing the type of the returned list I figured it's safest and easiest to just shove the member_id back into the ServiceInstance. I chose to make this part of the iterator action because it should remain Group method agnostic should those change or more options be added. Because that happens automatically now we also need a way to assign a member_id if we manually join a new instance to our ServerSet object (i.e. we did not get the info from ZK).

[atollena]

@brutkin thinking more about it I'm not sure member_id should be serialized in the json blob, since it's unknown when joining the serverset. How are you going to make sure it matches the sequential id that is assigned by ZK? In makes me wonder whether adding member id to ServiceInstance is the right solution for what you're trying to do. Couldn't you just store a mapping of member IDs to ServiceInstance at call site? What prompts adding this to ServiceInstance?

[atollena]

Sorry, I posted before seeing your latest answer, which addresses my questions.

[dogbunny]

Are you suggestion I add it in twitter.common.zookeeper.group.group and twitter.common.zookeeper.group.kazoo_group instead? I think I'd still need most of these changes as well but that would likely work.

[dogbunny]

I didn't see a good way to add the member_id to the group.info() methods because they return a captured callback on a Future. The method also returns data as a json string (or thrift) so adding member_id to the blob before passing it to unpack() is more trouble than it's worth.

The good news is that ServerSet.join does not need to handle member_id at all since that's handled by the group.join method which also handles the member serialization. I still have to add the optional member_id params to the ServiceInstance functions (including init for testing)

[dogbunny]

Not sure if test_sampler_base is flaky but the failure definitely had nothing to do with my changes: [1m test_clock.tick(5)  assert test_clock.converge(threads=[sampler]) > assert sampler.count == 6 E assert 2 == 6 E + where 2 = <TestSampler(Thread-1 [TID=5876], started daemon 140429632481024)>.count

                 tests/python/twitter/common/metrics/test_sampling.py:58: AssertionError

[Yasumoto]

This is more me being a n00b than anything else, is this the same as Sequence Nodes as listed at https://zookeeper.apache.org/doc/trunk/zookeeperProgrammers.html ? Regardless, this change seems to make sense, and having a mapping of instance # to compare with separate member IDs would definitely be helpful and make sense.

Thanks @brutkin! 🚢

[dogbunny]

@Yasumoto, that's exactly right. In our case it's member_ which lets you know if you have duplicate members in the pool. Serverset clients consolidate duplicate ServiceInstances so it shouldn't affect behavior but it does indicate something went wrong - generally problems around leader election but could possibly be bad client or even flaky network - and makes it much easier to track those down.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[grimreaper]

Thank you for your contribution. Unfortunately, we’re not going to continue maintaining twitter-commons and are archiving all pull requests.