search

docker / libkv

Distributed key/value store abstraction library
Apache License 2.0
853 stars 205 forks source link

stop zk watches cleanly #129

Closed talbright closed 8 years ago

talbright commented 8 years ago
talbright commented 8 years ago

Seems odd etcd tests would be failing, is master stable? Will have to look into it.

abronan commented 8 years ago

Hi @talbright, it's definitely not related to your PR.

Looking at the trace, this seems like a delete event (lock release) was triggered right when the check for Get happens with etcd but pair then is nil. From the travis history it looks like the tests have been running consistently slower and slower for the past few months, so seems like a odd timing issue. Will look into improving the tests to make it more reliable (like having a clear path and not triggering the nil pointer for the cleanup to happen properly between tests).

talbright commented 8 years ago

Thanks for the confirmation! I closed this PR after reading the contributor guidelines that suggested it was favorable to separate library upgrades/changes from taking advantage of any new features they might bring in. See #130

Some food for thought:

Timing issues can really plague projects like this and go-zookeeper, its the nature of the beast to some extent. I've cleaned up a lot of the timing related bugs in my fork of go-zookeeper, but there's still some outstanding that can happen (I went from 9 of 10 builds failing to closer to 9 of 10 builds passing.) In our internal projects we use (ginkgo)[https://github.com/onsi/ginkgo], and have found the asynchronous matchers in gomega very helpful.

I know there is a general stance against using heavy testing frameworks in the contribution guidelines, not to mention the amount of work it would take to switch tests over. However, there might be something useful to glean from the implementation of http://onsi.github.io/gomega/#eventually and some of the other asynchronous matchers.