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

Etcd service watcher #58

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

Conversation

bobtfish
Copy link
Contributor

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


require 'etcd'

# Monkeypatch till 91f9e72d6d57ae3760e9266835f404d986072590 gets to rubygems..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SEJeff
Copy link

SEJeff commented Aug 21, 2014

@bobtfish mind rebasing this to merge cleanly with master?

@tcolgate
Copy link

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).

@0xgeert
Copy link

0xgeert commented Nov 9, 2014

any progress on this?

@bobtfish
Copy link
Contributor Author

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?)

@0xgeert
Copy link

0xgeert commented Nov 10, 2014

@bobtfish Cheers. Hope this will be merged soon.

@bobtfish
Copy link
Contributor Author

On Nov 18, 2014, at 8:54 AM, Zane [email protected] 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
Copy link

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 [email protected] wrote:

On Nov 18, 2014, at 8:54 AM, Zane [email protected] 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
Copy link

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
Copy link

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
Copy link

konsti commented Dec 22, 2014

@igor47 Any change this is merged with master?

@Zolmeister
Copy link

status?

@SEJeff
Copy link

SEJeff commented Feb 6, 2015

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

@pgasiorowski
Copy link

Any update on this?

@tcolgate
Copy link

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

On 19 February 2015 at 08:35, Piotr Gasiorowski [email protected]
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
Copy link
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
Copy link

+1

@jolynch
Copy link
Collaborator

jolynch commented Sep 7, 2015

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

@bobtfish bobtfish force-pushed the etcd_service_watcher branch 2 times, most recently from af844e4 to bff24d2 Compare September 23, 2015 04:11
@bobtfish
Copy link
Contributor Author

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


require 'etcd'

module Synapse
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be class Synapse::ServiceWatcher

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

@jolynch
Copy link
Collaborator

jolynch commented Nov 3, 2015

@bobtfish ping?

@jolynch
Copy link
Collaborator

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
Copy link

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.