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

Already on GitHub? Sign in to your account

Move archaius bindings here #25

Merged
merged 6 commits into from Mar 27, 2013

Conversation

Projects
None yet
2 participants
Contributor

Kami commented Mar 26, 2013

This is primarily for consistency reasons (Curator bindings already live in this repository).

I see us eventually moving Curator and Archaius outside of this into a separate repository (it should be relatively easy and painless) again.

@gdusbabek gdusbabek and 1 other commented on an outdated diff Mar 27, 2013

service-registry-archaius/pom.xml
+ </dependency>
+ <dependency>
+ <groupId>com.netflix.archaius</groupId>
+ <artifactId>archaius-core</artifactId>
+ <version>0.5.3</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
+ <dependency>
+ <groupId>com.rackspacecloud</groupId>
+ <artifactId>service-registry-client</artifactId>
+ <version>1.0.0-SNAPSHOT</version>
@gdusbabek

gdusbabek Mar 27, 2013

Contributor

is that version correct?

@Kami

Kami Mar 27, 2013

Contributor

Yes, there is a different PR (based on this one) which bumps it to 2.0.0 and updates the affected code.

@gdusbabek gdusbabek and 1 other commented on an outdated diff Mar 27, 2013

...lix/config/sources/ServiceRegistryClientProvider.java
@@ -0,0 +1,24 @@
+package com.netflix.config.sources;
+
+import com.rackspacecloud.client.service_registry.Client;
+import com.rackspacecloud.client.service_registry.objects.ConfigurationValue;
+import com.rackspacecloud.client.service_registry.objects.Service;
+
+import java.util.List;
+
+public class ServiceRegistryClientProvider implements ServiceRegistryClient {
+ private final Client client;
+
+ public ServiceRegistryClientProvider(String user, String key, String region) {
+ client = new Client(user, key, region);
+ }
+ @Override
@gdusbabek

gdusbabek Mar 27, 2013

Contributor

you probably care about this more than me: newline.

@Kami

Kami Mar 27, 2013

Contributor

I do, but I avoid doing most of the "style" stuff in this pr

@gdusbabek gdusbabek commented on an outdated diff Mar 27, 2013

...fig/sources/ServiceRegistryConfigurationProvider.java
+
+ Map<String, Object> map = new HashMap<String, Object>();
+ for (ConfigurationValue value : client.getConfiguration()) {
+ map.put(value.getId(), value.getValue());
+ }
+
+ // Do a query to get all the service tags we're interested in,
+ // once we have that we'll namespace the parameters
+ String tags = dynamicServiceTags.get();
+ if (!tags.isEmpty()) {
+ Set<String> serviceTags = new HashSet<String>(Arrays.asList(tags.split(SEPARATOR)));
+ for (String tag: serviceTags) {
+ Set<InetSocketAddress> pairs = new HashSet<InetSocketAddress>();
+ String key = PREFIX + DELIMITER + tag + DELIMITER + SUFFIX;
+ for (Service service : client.getServices(tag)) {
+ pairs.add(getHostPortPair(service));
@gdusbabek

gdusbabek Mar 27, 2013

Contributor

getHostPortPair() can return null. This will be bad for pairs.add(). It is probably best to use a local and do a null check.

@gdusbabek gdusbabek commented on the diff Mar 27, 2013

...fig/sources/ServiceRegistryConfigurationProvider.java
+ }
+ }
+ return PollResult.createFull(map);
+ }
+
+ @Override
+ public String toString() {
+ return "ServiceRegistryConfigurationProvider [client=" + client.toString() + "]";
+ }
+
+ /**
+ * Safely get the information from our convention
+ * @param svc
+ * @return
+ */
+ public static InetSocketAddress getHostPortPair(Service svc) {
@gdusbabek

gdusbabek Mar 27, 2013

Contributor

Is the premise that users will not query for a tag that does not have the appropriate metdata? If so, then I think it is ok to log at ERROR. But if there is no such premise, we ought to log at DEBUG.

@Kami

Kami Mar 27, 2013

Contributor

That's an old stuff which I want to avoid touching in this PR.

This one is just moving it into the same repository.

Contributor

Kami commented Mar 27, 2013

@gdusbabek Opened a separate PR for 2.0.0 upgrades and style changes and improvements which is based on this branch - #29

@Kami Kami added a commit that referenced this pull request Mar 27, 2013

@Kami Kami Merge pull request #25 from racker/move_archaius_bindings_here
Move archaius bindings here
efb95af

@Kami Kami merged commit efb95af into master Mar 27, 2013

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