From 983668ffbdca28513e934bb45d265e3a9501f8e2 Mon Sep 17 00:00:00 2001 From: Scott Leberknight <174812+sleberknight@users.noreply.github.com> Date: Sun, 17 Dec 2023 11:26:26 -0500 Subject: [PATCH] Log warning if Consul failover interceptor is set multiple times (#308) * Log warning if Consul failover interceptor is set multiple times * Log a warning in Consul if both withMultipleHostAndPort and withFailoverInterceptor are called when building a Consul instance. * This is important because calling them both results in a "last one wins" scenario, since the last method called sets the failover interceptor. Closes #306 * Update javadocs regarding overriding failover interceptors --- .../java/org/kiwiproject/consul/Consul.java | 32 ++++++++++++++++++ .../org/kiwiproject/consul/ConsulTest.java | 33 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/main/java/org/kiwiproject/consul/Consul.java b/src/main/java/org/kiwiproject/consul/Consul.java index 4bf4ebf..4453bd8 100644 --- a/src/main/java/org/kiwiproject/consul/Consul.java +++ b/src/main/java/org/kiwiproject/consul/Consul.java @@ -25,6 +25,8 @@ import org.kiwiproject.consul.util.bookend.ConsulBookendInterceptor; import org.kiwiproject.consul.util.failover.ConsulFailoverInterceptor; import org.kiwiproject.consul.util.failover.strategy.ConsulFailoverStrategy; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import retrofit2.Retrofit; import retrofit2.converter.jackson.JacksonConverterFactory; @@ -49,6 +51,8 @@ */ public class Consul { + private static final Logger LOG = LoggerFactory.getLogger(Consul.class); + /** * Default Consul HTTP API host. */ @@ -318,6 +322,7 @@ public static class Builder { private Interceptor headerInterceptor; private Interceptor consulBookendInterceptor; private Interceptor consulFailoverInterceptor; + private int numTimesConsulFailoverInterceptorSet; private final NetworkTimeoutConfig.Builder networkTimeoutConfigBuilder = new NetworkTimeoutConfig.Builder(); private ExecutorService executorService; private ConnectionPool connectionPool; @@ -500,6 +505,11 @@ public Builder withHostAndPort(HostAndPort hostAndPort) { * Sets the list of hosts to contact if the current request target is * unavailable. When the call to a particular URL fails for any reason, the next {@link HostAndPort} specified * is used to retry the request. This will continue until all urls are exhausted. + *

+ * Internally, this method constructs a {@link ConsulFailoverInterceptor} with a + * {@link org.kiwiproject.consul.util.failover.strategy.BlacklistingConsulFailoverStrategy BlacklistingConsulFailoverStrategy}. + * If you call this method, you should not use {@link #withFailoverInterceptor(ConsulFailoverStrategy)} + * as it will create a new failover interceptor which overrides the one set by this method. * * @param hostAndPort A collection of {@link HostAndPort} that define the list of Consul agent addresses to use. * @param blacklistTimeInMillis The timeout (in milliseconds) to blacklist a particular {@link HostAndPort} before trying to use it again. @@ -508,8 +518,10 @@ public Builder withHostAndPort(HostAndPort hostAndPort) { public Builder withMultipleHostAndPort(Collection hostAndPort, long blacklistTimeInMillis) { checkArgument(blacklistTimeInMillis >= 0, "Blacklist time must be positive"); checkArgument(hostAndPort.size() >= 2, "Minimum of 2 addresses are required"); + logWarningIfConsulFailoverInterceptorAlreadySet("withMultipleHostAndPort"); consulFailoverInterceptor = new ConsulFailoverInterceptor(hostAndPort, blacklistTimeInMillis); + ++numTimesConsulFailoverInterceptorSet; withHostAndPort(hostAndPort.stream().findFirst().orElseThrow()); return this; @@ -517,16 +529,36 @@ public Builder withMultipleHostAndPort(Collection hostAndPort, long /** * Constructs a failover interceptor with the given {@link ConsulFailoverStrategy}. + *

+ * If you call this method, you should not use {@link #withMultipleHostAndPort(Collection, long)} + * as it will create a new failover interceptor which overrides the one set by this method. + * * @param strategy The strategy to use. * @return The builder. */ public Builder withFailoverInterceptor(ConsulFailoverStrategy strategy) { checkArgument(nonNull(strategy), "Must not provide a null strategy"); + logWarningIfConsulFailoverInterceptorAlreadySet("withFailoverInterceptor"); consulFailoverInterceptor = new ConsulFailoverInterceptor(strategy); + ++numTimesConsulFailoverInterceptorSet; return this; } + private void logWarningIfConsulFailoverInterceptorAlreadySet(String methodName) { + if (numTimesConsulFailoverInterceptorSet > 0) { + LOG.warn("A ConsulFailoverInterceptor was already present; this invocation to '{}' overrides it!" + + " Make sure either 'withMultipleHostAndPort' or 'withFailoverInterceptor' is called," + + " but not both.", + methodName); + } + } + + @VisibleForTesting + int numTimesConsulFailoverInterceptorSet() { + return numTimesConsulFailoverInterceptorSet; + } + /** * Sets the URL from a string. * diff --git a/src/test/java/org/kiwiproject/consul/ConsulTest.java b/src/test/java/org/kiwiproject/consul/ConsulTest.java index 973f197..05fd820 100644 --- a/src/test/java/org/kiwiproject/consul/ConsulTest.java +++ b/src/test/java/org/kiwiproject/consul/ConsulTest.java @@ -1,7 +1,9 @@ package org.kiwiproject.consul; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.mock; import com.google.common.net.HostAndPort; import org.junit.jupiter.api.DisplayName; @@ -11,6 +13,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; +import org.kiwiproject.consul.util.failover.strategy.ConsulFailoverStrategy; import java.util.Collection; import java.util.List; @@ -59,4 +62,34 @@ void shouldRequireFailoverStrategy() { .withMessage("Must not provide a null strategy"); } } + + @Nested + class WithMultipleFailoverInterceptors { + + @Test + void shouldDetectWhenConsulInterceptorAlreadySetBy_withMultipleHostAndPort() { + var hosts = List.of( + HostAndPort.fromString("consul1.acme.com:8500"), + HostAndPort.fromString("consul2.acme.com:8500") + ); + var consulBuilder = Consul.builder() + .withMultipleHostAndPort(hosts, 10_000) + .withFailoverInterceptor(mock(ConsulFailoverStrategy.class)); + + assertThat(consulBuilder.numTimesConsulFailoverInterceptorSet()).isEqualTo(2); + } + + @Test + void shouldDetectWhenConsulInterceptorAlreadySetBy_withFailoverInterceptor() { + var hosts = List.of( + HostAndPort.fromString("consul1.acme.com:8500"), + HostAndPort.fromString("consul2.acme.com:8500") + ); + var consulBuilder = Consul.builder() + .withFailoverInterceptor(mock(ConsulFailoverStrategy.class)) + .withMultipleHostAndPort(hosts, 7_500); + + assertThat(consulBuilder.numTimesConsulFailoverInterceptorSet()).isEqualTo(2); + } + } }