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

Etcd service watcher #58

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@bobtfish
Contributor

bobtfish commented Apr 13, 2014

And the etcd watcher part to go with the reporter :)

@calebbrown calebbrown referenced this pull request Aug 21, 2014

Open

etcd service watcher #35

require 'etcd'
# Monkeypatch till 91f9e72d6d57ae3760e9266835f404d986072590 gets to rubygems..

This comment has been minimized.

@calebbrown

calebbrown Aug 21, 2014

0.2.4 was just released and is available in Ruby Gems - so this could probably be cleaned up.

@SEJeff

This comment has been minimized.

SEJeff commented Aug 21, 2014

@bobtfish mind rebasing this to merge cleanly with master?

@bobtfish bobtfish force-pushed the bobtfish:etcd_service_watcher branch from 276fceb to 3988e57 Aug 24, 2014

@tcolgate

This comment has been minimized.

tcolgate commented Oct 13, 2014

Is this likely to merged soon? The nerve part already seems to be merged? etcd does seem like a better choice (nicer cli tools,and less of a total collapse if quorum is lost).

@gebrits

This comment has been minimized.

gebrits commented Nov 9, 2014

any progress on this?

@bobtfish

This comment has been minimized.

Contributor

bobtfish commented Nov 10, 2014

AFAIK this is good to merge, and it's waiting on someone from airbnb to actually merge it, rather than me to do anything to make it mergeable. (If not, I've missed something - please point it out?)

@gebrits

This comment has been minimized.

gebrits commented Nov 10, 2014

@bobtfish Cheers. Hope this will be merged soon.

@bobtfish

This comment has been minimized.

Contributor

bobtfish commented Nov 19, 2014

On Nov 18, 2014, at 8:54 AM, Zane notifications@github.com wrote:

Greetings -

I've implemented this watcher, but I am running into an error:

E, [2014-11-18T08:51:02.005658 #7449] ERROR -- Synapse::EtcdWatcher: synapse: invalid data in etcd node # at /service: 757: unexpected token at 'running' DATA running

For all entries in my etcd instance I get a return from synapse noted above.

Can you also show us what’s in etcd? (The key layout, and JSON contents)

I’m guessed that there is some somehow bad data that’s crashing things - if you can show me what it is, I’ll fix and add a test :)

Cheers
Tom

@tcolgate

This comment has been minimized.

tcolgate commented Nov 19, 2014

For what it is worth, I've done some additional work on the etcd watcher,
to include the failover host support, and add some retry logic. I've not
tidied it up enough for a PR yet but code is on master here:

https://github.com/we7/synapse

On 19 November 2014 11:59, Tomas Doran notifications@github.com wrote:

On Nov 18, 2014, at 8:54 AM, Zane notifications@github.com wrote:

Greetings -

I've implemented this watcher, but I am running into an error:

E, [2014-11-18T08:51:02.005658 #7449] ERROR -- Synapse::EtcdWatcher:
synapse: invalid data in etcd node # at /service: 757: unexpected token at
'running' DATA running

For all entries in my etcd instance I get a return from synapse noted
above.

Can you also show us what’s in etcd? (The key layout, and JSON contents)

I’m guessed that there is some somehow bad data that’s crashing things -
if you can show me what it is, I’ll fix and add a test :)

Cheers
Tom


Reply to this email directly or view it on GitHub
#58 (comment).

Tristan Colgate-McFarlane

"You can get all your daily vitamins from 52 pints of guiness, and a
glass of milk"

@sepulworld

This comment has been minimized.

sepulworld commented Nov 20, 2014

Thanks Tom. Yes, I wasn't using Nerve to populate host information in Etcd so the key values didn't match up with what the Etcd watcher on synapse was looking for. I am trying to switch over to using Nerve but etcd support doesn't seem to be a part of the current Gem version and doing a gem install_specific against the github repo doesn't work either. Do we know when the etcd functionality for Nerve will be stable?

@sepulworld

This comment has been minimized.

sepulworld commented Nov 22, 2014

Hi Tom,

Here is my etcd key layout:

curl http://192.168.183.171:4001/v2/keys/service

{"action":"get","node":{"key":"/service","dir":true,"nodes":[{"key":"/service/192.168.186.158:49234","value":"running","expiration":"2014-11-22T21:17:37.738052203Z","ttl":18,"modifiedIndex":73811,"createdIndex":73811},{"key":"/service/192.168.186.158:49235","value":"running","expiration":"2014-11-22T21:17:38.321944617Z","ttl":18,"modifiedIndex":73812,"createdIndex":73812},{"key":"/service/192.168.186.158:49231","value":"running","expiration":"2014-11-22T21:17:36.733670014Z","ttl":17,"modifiedIndex":73809,"createdIndex":73809},{"key":"/service/192.168.186.158:49233","value":"running","expiration":"2014-11-22T21:17:36.878094294Z","ttl":17,"modifiedIndex":73810,"createdIndex":73810},{"key":"/service/192.168.186.158:49232","value":"running","expiration":"2014-11-22T21:17:36.517818371Z","ttl":17,"modifiedIndex":73808,"createdIndex":73808}],"modifiedIndex":3

@konsti

This comment has been minimized.

konsti commented Dec 22, 2014

@igor47 Any change this is merged with master?

@Zolmeister

This comment has been minimized.

Zolmeister commented Feb 5, 2015

status?

@SEJeff

This comment has been minimized.

SEJeff commented Feb 6, 2015

Well this has merge conflicts, so at a minimum needs to be rebased before it is merged.

@bobtfish bobtfish force-pushed the bobtfish:etcd_service_watcher branch from 8941208 to 15bfb29 Feb 9, 2015

@WooDzu

This comment has been minimized.

WooDzu commented Feb 19, 2015

Any update on this?

@tcolgate

This comment has been minimized.

tcolgate commented Feb 19, 2015

I've dropped my version of this PR. We've migrated to consul.

On 19 February 2015 at 08:35, Piotr Gasiorowski notifications@github.com
wrote:

Any update on this?


Reply to this email directly or view it on GitHub
#58 (comment).

Tristan Colgate-McFarlane

"You can get all your daily vitamins from 52 pints of guiness, and a
glass of milk"

@clizzin

This comment has been minimized.

Contributor

clizzin commented Feb 19, 2015

Sorry folks, the project maintainer @igor47 is really busy with other projects for Airbnb these days. We're doing our best to find time to respond to pull requests, and we're sorry contributors are waiting so long. Please bear with us in the meantime.

@apennebaker-ni

This comment has been minimized.

apennebaker-ni commented Apr 27, 2015

+1

@jolynch

This comment has been minimized.

Contributor

jolynch commented Sep 7, 2015

@bobtfish can you get a clean diff together and I can review/merge it?

@bobtfish bobtfish force-pushed the bobtfish:etcd_service_watcher branch 2 times, most recently from af844e4 to bff24d2 Sep 23, 2015

@bobtfish bobtfish force-pushed the bobtfish:etcd_service_watcher branch from bff24d2 to 5fc104f Sep 23, 2015

@bobtfish

This comment has been minimized.

Contributor

bobtfish commented Sep 23, 2015

@jolynch - how do you feel about this? Pulled in #86 also :)

require 'etcd'
module Synapse

This comment has been minimized.

@jolynch

jolynch Sep 24, 2015

Contributor

This should be class Synapse::ServiceWatcher

Also can you add this watcher to the auto creation test?

@@ -21,6 +22,8 @@ GEM
archive-tar-minitar
excon (>= 0.28)
json
etcd (0.2.4)

This comment has been minimized.

@jolynch

jolynch Sep 24, 2015

Contributor

So while testing this I came across quite a pickle, which is that the etcd ruby gem < 0.3.0 cannot handle etcd 2.0+ (it errors all over the place regarding 404 errors)

ranjib/etcd-ruby#51 has been merged and my local testing indicates that version 0.3.0 seems to fix things.

@bobtfish do you think we should depend on an old etcd or the new one? I sorta prefer the new one but I'm not sure what the community progress on moving to etcd 2.0 is like?

This comment has been minimized.

@jolynch

jolynch Oct 6, 2015

Contributor

Now that we recommend installing via gem, can we just do ~> 0.2 ?

def watch
while !@should_exit
begin
@etcd.watch(@discovery['path'], :timeout => 60, :recursive => true)

This comment has been minimized.

@jolynch

jolynch Sep 24, 2015

Contributor

Alright so while testing I came across this gem. If my understanding is correct etcd makes us not only do our own heartbeats from nerve (hella writes), but we also end up basically constantly polling in this function because there is no capability to filter watch events?

Basically without etcd-io/etcd#633 or etcd-io/etcd#174 closed SmartStack scalability on etcd will be limited to ... not very much.

If that understanding is correct can we implement get children watches internally, where we do a lightweight "see if the list of children changed" operation and if that is different we actually go through loading all the data (aka run discover). This way we're at least not constantly pulling all of the etcd state?

@jolynch

This comment has been minimized.

Contributor

jolynch commented Nov 3, 2015

@bobtfish ping?

@jolynch

This comment has been minimized.

Contributor

jolynch commented Oct 23, 2016

Looks like the v3 etcd API may have solved some of the fundamental issues. I'll take another look at this in the next few days.

@philips

This comment has been minimized.

philips commented Oct 23, 2016

@jolynch if you have any problems/questions feel free to ping https://groups.google.com/forum/#!forum/etcd-dev or @heyitsanthony

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment