Add lease recipe. #176
Add lease recipe. #176
Conversation
|
This commit looks perfectly fine, though I'm wary of adding more recipes to the core that the Kazoo maintainers don't have time to support (having recipes in kazoo gives people the impression we'll maintain them and handle bugs, per #175 which refers to a bug in a recipe that we merged). Also see #136 as it was decided not to include that. At the same time it would be sad if others were not able to benefit from this as its a very nice contribution. Rather than having a contrib/ type thing with 'unsupported' recipes, would one or two folks behind this PR be interested in being the owner for this recipe? (I'm mainly envisioning someone that could review changes/bugs reported solely in this recipe). |
|
I'm happy to volunteer as maintainer for this file. Let me know if you want me to find another for redundancy. It is ironic that you mentioned #136, as I was recently looking for a queue that removes duplicates, for job scheduling purposes. If it had been in kazoo, I would have found it and used it, maintained or not. The kazoo brand helps users find the implementations. If I put up a personal repository with my own recipes, they will go unnoticed. One could of course envision alternatives for using the brand value, such as a kazoo contrib page with links to third party recipes. |
|
Agreed, I think having 'owners' for recipes that land would help, as well as maybe some meta information in the module docs indicating the recipe's status, who the owner is, and if an owner decides to not maintain it then that could be indicated (no owner). |
|
I'm going to update the existing recipe's with maintenance/owner info so you can see the layout, and update the PR, then I'll be happy to merge it. Thanks for volunteering this and to own it! |
|
SGTM. Let me know when you have done so, and I'll update the PR. Thanks! On Wed, Mar 12, 2014 at 8:52 PM, Ben Bangert notifications@github.comwrote:
|
|
I've added maintainer data for the recipes I'm aware of. You can update this to indicate the maintainer and your comfort level with its status (production, beta, alpha, etc). |
|
Great, thanks! I added the maintainer information. I put the status as beta, even though the code is in production, since it is used on a non-critical path, and hasn't received much field testing yet. I'll check if a colleague wants to be co-maintainer as well. |
|
Also, looks like you're missing a doc/ page and entry for it so the docs can be built. |
|
Sorry for dropping this for so long. I pushed a doc entry, and the docs seem to build fine and look reasonable. I also read the (updated?) contribution instructions more carefully and added an entry in CHANGES. Travis reports a test error, but in a test that does not touch the recipe, and seems to be unrelated. If you prefer not to merge due to failures, I'll babysit and see if the build returns to green. |
| lock = client.Lock(path, ident) | ||
| try: | ||
| with lock: | ||
| now = utcnow() |
There was a problem hiding this comment.
Is there anyway to avoid using localtime (local to the client) here in a client and use the zookeeper node's time instead? Maybe just put a delta (or duration) in the znode and then use the znodes times? Just a suggestion that would avoid later timing issues.
There was a problem hiding this comment.
I think you have a valid theoretical point, but I'm not sure it is practical. IIUC, the advantage of using zk node time would be to handle scenarios when client clocks are widely out of sync. It is rare these days, since machines typically have NTP enabled by default, and if you are out of sync by minutes, you usually have worse trouble anyway. :-)
The disadvantages would be more complex code, and complex testing. I cannot think of an efficient way to test such a setup. Does zk provide remote controlled mock clock behaviour? Even if it does, it would become quite messy.
I should document the assumption that clocks are in sync, though. Will do.
|
r+, just needs a rebase to land |
|
@lallea ping, this looks good, just need a rebase for merging. |
|
Thanks for the ping. Now rebased, tests pass. |
This is in production at Spotify for monitoring scripts.