More efficient ServerListFilter.

Discovery now uses /v1/health/service/<serviceId> rather than /v1/catalog/service/<serviceId> which provides health checks.

HealthServiceServerListFilter now uses the health check to return only PASSING servers.

This eliminates extra calls to consul.

fixes gh-116

.gitignore

@@ -12,4 +12,4 @@ _site/
 *.iml
 .factorypath
 *.swp
-consul
+/consul

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulDiscoveryClient.java

@@ -16,28 +16,28 @@
 
 package org.springframework.cloud.consul.discovery;
 
-import static org.springframework.cloud.consul.discovery.IpAddressUtils.getCatalogServiceHost;
-
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
-import org.springframework.boot.autoconfigure.web.ServerProperties;
-import org.springframework.cloud.client.DefaultServiceInstance;
-import org.springframework.cloud.client.ServiceInstance;
-import org.springframework.cloud.client.discovery.DiscoveryClient;
-import org.springframework.util.StringUtils;
-
 import com.ecwid.consul.v1.ConsulClient;
 import com.ecwid.consul.v1.QueryParams;
 import com.ecwid.consul.v1.Response;
 import com.ecwid.consul.v1.agent.model.Member;
 import com.ecwid.consul.v1.agent.model.Self;
 import com.ecwid.consul.v1.agent.model.Service;
-import com.ecwid.consul.v1.catalog.model.CatalogService;
+import com.ecwid.consul.v1.health.model.HealthService;
+
+import org.springframework.boot.autoconfigure.web.ServerProperties;
+import org.springframework.cloud.client.DefaultServiceInstance;
+import org.springframework.cloud.client.ServiceInstance;
+import org.springframework.cloud.client.discovery.DiscoveryClient;
+import org.springframework.util.StringUtils;
 
 import lombok.extern.apachecommons.CommonsLog;
 
+import static org.springframework.cloud.consul.discovery.ConsulServerUtils.findHost;
+
 /**
  * @author Spencer Gibb
  */
@@ -52,8 +52,7 @@ public class ConsulDiscoveryClient implements DiscoveryClient {
 	private ServerProperties serverProperties;
 
 	public ConsulDiscoveryClient(ConsulClient client, ConsulLifecycle lifecycle,
-								 ConsulDiscoveryProperties properties,
-								 ServerProperties serverProperties) {
+			ConsulDiscoveryProperties properties, ServerProperties serverProperties) {
 		this.client = client;
 		this.lifecycle = lifecycle;
 		this.properties = properties;
@@ -72,7 +71,7 @@ public class ConsulDiscoveryClient implements DiscoveryClient {
 		String serviceId;
 		Integer port;
 		if (service == null) {
-			//possibly called before registration
+			// possibly called before registration
 			log.warn("Unable to locate service in consul agent: "
 					+ lifecycle.getServiceId());
 
@@ -81,7 +80,8 @@ public class ConsulDiscoveryClient implements DiscoveryClient {
 			if (port == 0 && serverProperties.getPort() != null) {
 				port = serverProperties.getPort();
 			}
-		} else {
+		}
+		else {
 			serviceId = service.getId();
 			port = service.getPort();
 		}
@@ -91,7 +91,8 @@ public class ConsulDiscoveryClient implements DiscoveryClient {
 		if (member != null) {
 			if (properties.isPreferIpAddress()) {
 				host = member.getAddress();
-			} else if (StringUtils.hasText(member.getName())) {
+			}
+			else if (StringUtils.hasText(member.getName())) {
 				host = member.getName();
 			}
 		}
@@ -108,12 +109,12 @@ public class ConsulDiscoveryClient implements DiscoveryClient {
 	}
 
 	private void addInstancesToList(List<ServiceInstance> instances, String serviceId) {
-		Response<List<CatalogService>> services = client.getCatalogService(serviceId,
-				QueryParams.DEFAULT);
-		for (CatalogService service : services.getValue()) {
-			String host = getCatalogServiceHost(service);
-			instances.add(new DefaultServiceInstance(serviceId, host,
-					service.getServicePort(), false));
+		Response<List<HealthService>> services = client.getHealthServices(serviceId,
+				false, QueryParams.DEFAULT);
+		for (HealthService service : services.getValue()) {
+			String host = findHost(service);
+			instances.add(new DefaultServiceInstance(serviceId, host, service
+					.getService().getPort(), false));
 		}
 	}
 

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulPing.java

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2013-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.consul.discovery;
+
+import com.netflix.loadbalancer.IPing;
+import com.netflix.loadbalancer.Server;
+
+/**
+ * "Ping" Consul
+ * i.e. we dont do a real "ping". We just assume that the server is up if Consul says so
+ * @author Spencer Gibb
+ */
+public class ConsulPing implements IPing {
+	@Override
+	public boolean isAlive(Server server) {
+		boolean isAlive = true;
+
+		if (server != null && server instanceof ConsulServer) {
+			ConsulServer consulServer = (ConsulServer) server;
+			return consulServer.isPassingChecks();
+		}
+
+		return isAlive;
+	}
+}

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulRibbonClientConfiguration.java

@@ -16,27 +16,27 @@
 
 package org.springframework.cloud.consul.discovery;
 
-import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses;
-import static com.netflix.client.config.CommonClientConfigKey.EnableZoneAffinity;
-
 import javax.annotation.PostConstruct;
 
-import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.beans.factory.annotation.Value;
-import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
-import org.springframework.cloud.consul.discovery.filters.ServiceCheckServerListFilter;
-import org.springframework.context.annotation.Bean;
-import org.springframework.context.annotation.Configuration;
-
 import com.ecwid.consul.v1.ConsulClient;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.config.ConfigurationManager;
 import com.netflix.config.DynamicPropertyFactory;
 import com.netflix.config.DynamicStringProperty;
+import com.netflix.loadbalancer.IPing;
 import com.netflix.loadbalancer.Server;
 import com.netflix.loadbalancer.ServerList;
 import com.netflix.loadbalancer.ServerListFilter;
 
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+import static com.netflix.client.config.CommonClientConfigKey.DeploymentContextBasedVipAddresses;
+import static com.netflix.client.config.CommonClientConfigKey.EnableZoneAffinity;
+
 /**
  * Preprocessor that configures defaults for Consul-discovered ribbon clients. Such as:
  * <code>@zone</code>, NIWSServerListClassName, DeploymentContextBasedVipAddresses,
@@ -74,7 +74,13 @@ public class ConsulRibbonClientConfiguration {
 
 	@Bean
 	public ServerListFilter<Server> ribbonServerListFilter() {
-		return new ServiceCheckServerListFilter(client);
+		return new HealthServiceServerListFilter();
+	}
+
+	@Bean
+	@ConditionalOnMissingBean
+	public IPing ribbonPing() {
+		return new ConsulPing();
 	}
 
 	@PostConstruct

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServer.java

@@ -16,9 +16,10 @@
 
 package org.springframework.cloud.consul.discovery;
 
-import static org.springframework.cloud.consul.discovery.IpAddressUtils.getCatalogServiceHost;
+import static org.springframework.cloud.consul.discovery.ConsulServerUtils.findHost;
 
-import com.ecwid.consul.v1.catalog.model.CatalogService;
+import com.ecwid.consul.v1.health.model.Check;
+import com.ecwid.consul.v1.health.model.HealthService;
 import com.netflix.loadbalancer.Server;
 
 /**
@@ -27,17 +28,15 @@ import com.netflix.loadbalancer.Server;
 public class ConsulServer extends Server {
 
 	private final MetaInfo metaInfo;
-	private final String address;
-	private final String node;
+	private final HealthService service;
 
-	public ConsulServer(final CatalogService service) {
-		super(getCatalogServiceHost(service), service.getServicePort());
-		address = service.getAddress();
-		node = service.getNode();
+	public ConsulServer(final HealthService healthService) {
+		super(findHost(healthService), healthService.getService().getPort());
+		this.service = healthService;
 		metaInfo = new MetaInfo() {
 			@Override
 			public String getAppName() {
-				return service.getServiceName();
+				return service.getService().getService();
 			}
 
 			@Override
@@ -52,9 +51,11 @@ public class ConsulServer extends Server {
 
 			@Override
 			public String getInstanceId() {
-				return service.getServiceId();
+				return service.getService().getId();
 			}
 		};
+
+		setAlive(isPassingChecks());
 	}
 
 	@Override
@@ -62,11 +63,16 @@ public class ConsulServer extends Server {
 		return metaInfo;
 	}
 
-	public String getAddress() {
-		return address;
+	public HealthService getHealthService() {
+		return this.service;
 	}
 
-	public String getNode() {
-		return node;
+	public boolean isPassingChecks() {
+		for (Check check : this.service.getChecks()) {
+			if (check.getStatus() != Check.CheckStatus.PASSING) {
+				return false;
+			}
+		}
+		return true;
 	}
 }

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerList.java

@@ -23,7 +23,7 @@ import java.util.List;
 import com.ecwid.consul.v1.ConsulClient;
 import com.ecwid.consul.v1.QueryParams;
 import com.ecwid.consul.v1.Response;
-import com.ecwid.consul.v1.catalog.model.CatalogService;
+import com.ecwid.consul.v1.health.model.HealthService;
 import com.netflix.client.config.IClientConfig;
 import com.netflix.loadbalancer.AbstractServerList;
 
@@ -61,16 +61,29 @@ public class ConsulServerList extends AbstractServerList<ConsulServer> {
 		if (client == null) {
 			return Collections.emptyList();
 		}
-		String tag = this.properties.getServerListQueryTags().get(this.serviceId); // null is ok
-		Response<List<CatalogService>> response = client.getCatalogService(
-				this.serviceId, tag, QueryParams.DEFAULT);
+		String tag = getTag(); // null is ok
+		Response<List<HealthService>> response = client.getHealthServices(
+				this.serviceId, tag, false, QueryParams.DEFAULT);
 		if (response.getValue() == null || response.getValue().isEmpty()) {
 			return Collections.emptyList();
 		}
 		List<ConsulServer> servers = new ArrayList<>();
-		for (CatalogService service : response.getValue()) {
+		for (HealthService service : response.getValue()) {
 			servers.add(new ConsulServer(service));
 		}
 		return servers;
 	}
+
+	private String getTag() {
+		return this.properties.getServerListQueryTags().get(this.serviceId);
+	}
+
+	@Override
+	public String toString() {
+		final StringBuffer sb = new StringBuffer("ConsulServerList{");
+		sb.append("serviceId='").append(serviceId).append('\'');
+		sb.append(", tag=").append(getTag());
+		sb.append('}');
+		return sb.toString();
+	}
 }

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/ConsulServerUtils.java

@@ -16,31 +16,27 @@
 
 package org.springframework.cloud.consul.discovery;
 
-import java.io.IOException;
-import java.net.Inet4Address;
-import java.net.InetAddress;
-import java.net.NetworkInterface;
-import java.util.Enumeration;
-
-import lombok.SneakyThrows;
-import lombok.extern.apachecommons.CommonsLog;
+import com.ecwid.consul.v1.health.model.HealthService;
 
 import org.springframework.util.StringUtils;
 
-import com.ecwid.consul.v1.catalog.model.CatalogService;
+import lombok.extern.apachecommons.CommonsLog;
 
 /**
  * @author Spencer Gibb
  */
 @CommonsLog
-public class IpAddressUtils {
+public class ConsulServerUtils {
 
-	public static String getCatalogServiceHost(CatalogService service) {
-		if (StringUtils.hasText(service.getServiceAddress())) {
-			return service.getServiceAddress();
-		} else if (StringUtils.hasText(service.getAddress())) {
+	public static String findHost(HealthService healthService) {
+		HealthService.Service service = healthService.getService();
+		HealthService.Node node = healthService.getNode();
+
+		if (StringUtils.hasText(service.getAddress())) {
 			return service.getAddress();
+		} else if (StringUtils.hasText(node.getAddress())) {
+			return node.getAddress();
 		}
-		return service.getNode();
+		return node.getNode();
 	}
 }

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilter.java

@@ -0,0 +1,53 @@
+/*
+ * Copyright 2013-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.consul.discovery;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import com.netflix.loadbalancer.Server;
+import com.netflix.loadbalancer.ServerListFilter;
+
+import lombok.extern.apachecommons.CommonsLog;
+
+/**
+ * ServerList implementaion that filters ConsulServers based on if all their Health Checks are PASSING.
+ * @author Spencer Gibb
+ */
+@CommonsLog
+public class HealthServiceServerListFilter implements ServerListFilter<Server> {
+	@Override
+	public List<Server> getFilteredListOfServers(List<Server> servers) {
+		List<Server> filtered = new ArrayList<>();
+
+		for (Server server : servers) {
+			if (server instanceof ConsulServer) {
+				ConsulServer consulServer = (ConsulServer) server;
+
+				if (consulServer.isPassingChecks()) {
+					filtered.add(server);
+				}
+
+			} else {
+				log.debug("Unable to determine aliveness of server type "+server.getClass()+", "+server);
+				filtered.add(server);
+			}
+		}
+
+		return filtered;
+	}
+}

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/filters/AliveServerListFilter.java

@@ -33,6 +33,7 @@ import com.netflix.loadbalancer.ServerListFilter;
  * live in both).
  * @author nicu marasoiu on 10.03.2015.
  */
+@Deprecated
 public class AliveServerListFilter implements ServerListFilter<Server> {
 	private FilteringAgentClient filteringAgentClient;
 
@@ -46,7 +47,7 @@ public class AliveServerListFilter implements ServerListFilter<Server> {
 		List<Server> filteredServers = new ArrayList<>();
 		for (Server server : servers) {
 			ConsulServer consulServer = ConsulServer.class.cast(server);
-			if (liveNodes.contains(consulServer.getAddress())) {
+			if (liveNodes.contains(consulServer.getHealthService().getService().getAddress())) {
 				filteredServers.add(server);
 			}
 		}

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/filters/FilteringAgentClient.java

@@ -26,6 +26,7 @@ import org.springframework.cloud.consul.model.SerfStatusEnum;
 import com.ecwid.consul.v1.ConsulClient;
 import com.ecwid.consul.v1.agent.model.Member;
 
+@Deprecated
 public class FilteringAgentClient {
 
 	private static final int ALIVE_STATUS = SerfStatusEnum.StatusAlive.getCode();

spring-cloud-consul-discovery/src/main/java/org/springframework/cloud/consul/discovery/filters/ServiceCheckServerListFilter.java

@@ -28,6 +28,7 @@ import com.netflix.loadbalancer.ServerListFilter;
 /**
  * Created by nicu on 12.03.2015.
  */
+@Deprecated
 public class ServiceCheckServerListFilter implements ServerListFilter<Server> {
 
 	private ConsulClient client;

spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/HealthServiceServerListFilterTests.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright 2013-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.cloud.consul.discovery;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import com.ecwid.consul.v1.health.model.Check;
+import com.ecwid.consul.v1.health.model.HealthService;
+import com.netflix.loadbalancer.Server;
+
+import org.junit.Test;
+
+import static com.ecwid.consul.v1.health.model.Check.CheckStatus.*;
+import static org.junit.Assert.assertThat;
+import static org.hamcrest.Matchers.*;
+
+/**
+ * @author Spencer Gibb
+ */
+public class HealthServiceServerListFilterTests {
+
+	@Test
+	public void testGetFilteredListOfServers() {
+		HealthServiceServerListFilter filter = new HealthServiceServerListFilter();
+
+		ArrayList<Server> servers = new ArrayList<>();
+		servers.add(newServer(PASSING));
+		servers.add(newServer(PASSING));
+		servers.add(newServer(CRITICAL));
+		servers.add(newServer(WARNING));
+
+		List<Server> filtered = filter.getFilteredListOfServers(servers);
+		assertThat("wrong # of filtered servers", filtered, hasSize(2));
+	}
+
+	private ConsulServer newServer(Check.CheckStatus checkStatus) {
+		HealthService healthService = new HealthService();
+		HealthService.Node node = new HealthService.Node();
+		node.setAddress("nodeaddr"+checkStatus.name());
+		node.setNode("nodenode"+checkStatus.name());
+		healthService.setNode(node);
+		HealthService.Service service = new HealthService.Service();
+		service.setAddress("serviceaddr"+checkStatus.name());
+		service.setId("serviceid"+checkStatus.name());
+		service.setPort(8080);
+		service.setService("serviceservice"+checkStatus.name());
+		healthService.setService(service);
+		ArrayList<Check> checks = new ArrayList<>();
+		Check check = new Check();
+		check.setStatus(checkStatus);
+		checks.add(check);
+		healthService.setChecks(checks);
+		return new ConsulServer(healthService);
+	}
+}