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

watcher for Apache Aurora zookeeper announcements #104

Closed
wants to merge 1 commit into from

Conversation

benley
Copy link

@benley benley commented Dec 5, 2014

Aurora zookeeper serversets are comparable to what Nerve does, but Aurora uses a different json structure in the zookeeper nodes, and it has provisions for more than one advertised port for a service.

Review on Reviewable

Aurora zookeeper serversets are similar to what Nerve does, but Aurora
uses a different json structure in the zookeeper nodes, and it has
provisions for more than one advertised port for a service.

Change-Id: I55303bd68677706146dcfef267e625ffea1e80f3
benley referenced this pull request in larryweya/synapse Mar 4, 2015
@larryweya
Copy link
Contributor

@benley I like your implementation, allowing a custom port_name, since the rest is exactly the same, I'll merge yours into mine.

@StephanErb
Copy link

Anything missing to get this one merged?

@StephanErb
Copy link

@jolynch you have just merged a Marathon watcher (#101) for the Marathon Mesos Framework. This one implements the same for the Apache Aurora Mesos framework. Any change to get this one here merged as well? The code is rock solid; we are already using it in production.

@benley
Copy link
Author

benley commented Oct 10, 2015

I can update this to fit the new plugin model pretty easily if there's actually a chance of getting it merged.

@jolynch
Copy link
Collaborator

jolynch commented Oct 10, 2015

Yes I'm trying to work my way through the pull requests. If you pull/rebase I can review this one next. I think Igor is handling review of the exhibitor watcher, and we have a few more to review.

@StephanErb
Copy link

@benley two minor remarks:

  • This watcher does not only support Aurora but also others, such as finagle. Renaming it to serverset watcher would make that more apparent.
  • The pull request is missing a short description in the REAMDE https://github.com/airbnb/synapse#service-discovery

Really looking forward to seeing this merged. Thanks for your effort 👍

@benley
Copy link
Author

benley commented Oct 11, 2015

I agree about renaming it to serverset watcher; I'll get the PR updated soon.

# "port": 31943
# },
# "shard": 0,
# "status": "ALIVE"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we take into account the ALIVE status?

Choose a reason for hiding this comment

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

Looking at Aurora, I am not sure if the status is ever toggled. Aurora seems to simply remove the znode when the registered server disconnects. I'd say we leave it out for now.

@jolynch
Copy link
Collaborator

jolynch commented Oct 14, 2015

See #144 for the current deserialize interface

@benley
Copy link
Author

benley commented Oct 15, 2015

I'm planning on working on this PR tomorrow afternoon-ish, fwiw

@jolynch
Copy link
Collaborator

jolynch commented Dec 23, 2015

I believe that #153 should support what this PR requests via the new decode option to the ZookeeperWatcher. The README has details for how to configure it.

If that doesn't work please open and issue/PR and we can work on it :-)

@jolynch jolynch closed this Dec 23, 2015
@benley
Copy link
Author

benley commented Dec 23, 2015

Thanks for finishing this work, @jolynch! I got buried with other projects and totally lost track of it.

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.

4 participants