Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eternal watch should be robust when blocks are slow #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

lazyatom
Copy link

In order to ensure that we don't miss any key changes while we are doing work, the eternal_watch method should track the modifiedIndex attribute of the previous response and watch again using the next index value.

This pull request also adds specs for the other basic behaviours of eternal_watch, and fixes a minor but when it is called with only one argument.

This is dictated by the existing implementation of `watch`.
Before this method can become more sophisticated, it should be tested
a little. In these two specs, we exercise the basic
behaviour (watching for multiple changes to a key, instead of
returning after one) and that recursive keys can be watched.

The `eternal_watch` method uses `loop`, which would normally be hard
to test, but we can control it in the specs by breaking out of the
loop using an exception once we know we've exercised everything we
care about. I've added some code to help manage that in a simple way.
It's good practice for etcd watchers to track what the latest update
they received was, and then resume watching immediately after that
update, to ensure that no changes might've been missed between the
return of the first watch and the start of the second; i.e. when
actual work on the changed key/value is being done.

At the moment, `eternal_watch` doesn't do that, so if the yielded
block doesn't return before the next key update, that update will be
lost.

Subsequent commits should fix this test and make the behaviour of
`eternal_watch` more robust.
If the watched key changes while the given block executes, we will now
still pick up the change.
@ranjib
Copy link
Owner

ranjib commented Jan 17, 2018

@lazyatom this library is not actively maintained. Let me know if you would be interested in taking over maintainership

@lazyatom
Copy link
Author

It would probably be more pragmatic to work with @davissp14 to merge in his v3-compatible codebase here, and then use that as the basis for future development. I'd be happy to help facilitate that (including handing off ownership from you, if you like), if it's something you are both interested in?

@davissp14
Copy link

davissp14 commented Jan 17, 2018

My goal is to actually get these projects under the CoreOS account. I think this will be a better long term solution for both Etcd and for dealing with the inactive maintainer issue.

It would probably be more pragmatic to work with @davissp14 to merge in his v3-compatible codebase here, and then use that as the basis for future development.

I personally don't like the idea of munging the v2 and v3 codebases as they are and should be thought of as completely different databases. I think/hope that the duel support going on in the v3 binaries is a short-term solution to account for the immature v3 client driver ecosystem.

Relevant Issue:
etcd-io/etcd#9156

@lazyatom
Copy link
Author

My thinking was to branch this code as v2, add your code as master and start releasing your code as the etcd gem with a major version bump (maybe even up to v3) to indicate the API change. Anyone who needs to stick with the v2 API can continue sticking with the old code (and fixes can go on the v2 branch with releases under the current major gem version).

What I'd most like to see is that the "official" gem (either eventually under CoreOS' account, or semi-official if not) be able to use the etcd name on RubyGems.org, without breaking stuff for existing users (beyond locking up their version dependencies).

If @ranjib is happy to transfer/share gem ownership then we could achieve this without the codebases living in the same repository, but it might be confusing for users reporting issues if the codebases don't live together (albeit separated via git history and branches).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants