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

EtcdConfigurationSource #318

Merged
merged 3 commits into from Jun 18, 2015
Merged

Conversation

spoon16
Copy link

@spoon16 spoon16 commented Jun 12, 2015

An Etcd watched configuration source very similar to the ZooKeeperConfigurationSource.

Uses the Boon Etcd client https://github.com/boonproject/boon/blob/master/etcd/README.md. Seems to be the most widely adopted Etcd client.

Tests included.

@cloudbees-pull-request-builder

NetflixOSS » archaius » archaius-pull-requests #85 FAILURE
Looks like there's a problem with this pull request

@spoon16
Copy link
Author

spoon16 commented Jun 12, 2015

Build failed because of a JVM error... or maybe a bunch of failing unit tests unrelated to the Etcd module.

Unless I am missing something.

@cloudbees-pull-request-builder

archaius-pull-requests #254 SUCCESS
This pull request looks good

@howardyuan
Copy link
Contributor

There're two pull-requests builds. One of them was not configured with
proper Gradle opts so it runs out Permgen. I'll see how to either disable
the second one or add proper GRADLE_OPTS.

On Thu, Jun 11, 2015 at 6:02 PM, Eric Schoonover notifications@github.com
wrote:

Build failed because of a JVM error... or maybe a bunch of failing unit
tests unrelated to the Etcd module.

Unless I am missing something.


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

final Map<String, Object> set = Maps.newHashMap();
final Map<String, Object> delete = Maps.newHashMap();

final String action = updateResponse.action().toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the action() return null when it's error?

Copy link
Author

Choose a reason for hiding this comment

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

only if I duplicate the etcd.waitRecursive call on l:89

It'd probably be best to capture the wasError condition, throw an exception and have a finally that calls etcd.waitRecursive. But I'm not sure that really makes things more readable. I'll just duplicate l:89 and return early.

@cloudbees-pull-request-builder

NetflixOSS » archaius » archaius-pull-requests #86 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

archaius-pull-requests #255 SUCCESS
This pull request looks good

@spoon16
Copy link
Author

spoon16 commented Jun 12, 2015

@howardyuan, tried to address your feedback with the following changes.

  • fixed bug where delete wouldn't remove the value from the EtcdConfigurationSource
  • added a unit test to validate delete behavior
  • refactored the updateHandler in EtcdConfigurationSource... tried to improve readability by not always instantiating three maps

@howardyuan
Copy link
Contributor

Looks good to me. Will leave the PR open for a few days to see if anybody else have any more comments.

@spoon16
Copy link
Author

spoon16 commented Jun 18, 2015

@howardyuan merge?

howardyuan added a commit that referenced this pull request Jun 18, 2015
EtcdConfigurationSource -- @spoon16 Thanks for the contributions!
@howardyuan howardyuan merged commit 2db6b85 into Netflix:master Jun 18, 2015
@chandrapersonal
Copy link

why there is no case for node action "compareandswap" in etcd UpdateHandler. we have node actions create,set and delete action on UpdateHandler . but no compareandswap. Is there any reason for not having that action.

@spoon16 spoon16 deleted the etcd-source branch June 30, 2015 17:29
@spoon16
Copy link
Author

spoon16 commented Jun 30, 2015

@chandrapersonal I don't think "compareandswap" is a type of update notification. It's part of the Etcd update API and the side effect is that an update notification is emitted, either set or create.

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.

None yet

4 participants