From 152171b135d689df8c341d897fbc3e2a75a7da23 Mon Sep 17 00:00:00 2001
From: Daniel Wu <daniel.wu06@sap.com>
Date: Thu, 19 May 2022 10:48:33 +0800
Subject: [PATCH 1/5] allow key deletion to trigger configuration refresh
---
.../cloud/consul/config/ConfigWatch.java | 48 ++++++++-----------
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
index d28636dc3..174d143a6 100644
--- a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
+++ b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
@@ -157,34 +157,26 @@ public void watchConfigKeyValues() {
Response<List<GetValue>> response = this.consul.getKVValues(context, aclToken,
new QueryParams(this.properties.getWatch().getWaitTime(), currentIndex));
- // if response.value == null, response was a 404, otherwise it was a
- // 200, reducing churn if there wasn't anything
- if (response.getValue() != null && !response.getValue().isEmpty()) {
- Long newIndex = response.getConsulIndex();
-
- if (newIndex != null && !newIndex.equals(currentIndex)) {
- // don't publish the same index again, don't publish the first
- // time (-1) so index can be primed
- if (!this.consulIndexes.containsValue(newIndex) && !currentIndex.equals(-1L)) {
- if (log.isTraceEnabled()) {
- log.trace("Context " + context + " has new index " + newIndex);
- }
- RefreshEventData data = new RefreshEventData(context, currentIndex, newIndex);
- this.publisher.publishEvent(new RefreshEvent(this, data, data.toString()));
- }
- else if (log.isTraceEnabled()) {
- log.trace("Event for index already published for context " + context);
- }
- this.consulIndexes.put(context, newIndex);
- }
- else if (log.isTraceEnabled()) {
- log.trace("Same index for context " + context);
- }
- }
- else if (log.isTraceEnabled()) {
- log.trace("No value for context " + context);
- }
-
+ // if response.value == null, response was a 404, a key is deleted
+ Long newIndex = response.getConsulIndex();
+ if (newIndex != null && !newIndex.equals(currentIndex)) {
+ // don't publish the same index again, don't publish the first
+ // time (-1) so index can be primed
+ if (!this.consulIndexes.containsValue(newIndex) && !currentIndex.equals(-1L)) {
+ if (log.isTraceEnabled()) {
+ log.trace("Context " + context + " has new index " + newIndex);
+ }
+ RefreshEventData data = new RefreshEventData(context, currentIndex, newIndex);
+ this.publisher.publishEvent(new RefreshEvent(this, data, data.toString()));
+ }
+ else if (log.isTraceEnabled()) {
+ log.trace("Event for index already published for context " + context);
+ }
+ this.consulIndexes.put(context, newIndex);
+ }
+ else if (log.isTraceEnabled()) {
+ log.trace("Same index for context " + context);
+ }
}
catch (Exception e) {
// only fail fast on the initial query, otherwise just log the error
From 6f5558e6695f84cda78569c9dd5121b51825d313 Mon Sep 17 00:00:00 2001
From: Daniel Wu <daniel.wu06@sap.com>
Date: Sat, 21 May 2022 12:18:24 +0800
Subject: [PATCH 2/5] allow deletion trigger, and fix some check style
---
.../ConsulBinderConfigurationTests.java | 6 +++
.../binder/test/producer/TestProducer.java | 3 ++
.../cloud/consul/config/ConfigWatch.java | 38 +++++++++----------
.../consul/test/ConsulTestcontainers.java | 3 ++
.../discovery/ConsulHeartbeatTaskTests.java | 2 +-
...nsulAutoServiceRegistrationRetryTests.java | 6 +++
6 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/config/ConsulBinderConfigurationTests.java b/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/config/ConsulBinderConfigurationTests.java
index 3da4e448b..1284ab3f7 100644
--- a/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/config/ConsulBinderConfigurationTests.java
+++ b/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/config/ConsulBinderConfigurationTests.java
@@ -34,6 +34,9 @@
*/
public class ConsulBinderConfigurationTests {
+ /**
+ * exception
+ */
@Rule
public ExpectedException exception = ExpectedException.none();
@@ -55,6 +58,9 @@ public void consulDisabledDisablesBinder() {
interface Events {
+ /**
+ * @return the channel
+ */
// @Output
MessageChannel purchases();
diff --git a/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/test/producer/TestProducer.java b/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/test/producer/TestProducer.java
index e15a2a9e9..95fdaf4df 100644
--- a/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/test/producer/TestProducer.java
+++ b/spring-cloud-consul-binder/src/test/java/org/springframework/cloud/consul/binder/test/producer/TestProducer.java
@@ -93,6 +93,9 @@ public boolean partitionStrategyInvoked() {
public static class StubPartitionSelectorStrategy implements PartitionSelectorStrategy {
+ /**
+ * invoked
+ */
public volatile boolean invoked = false;
@Override
diff --git a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
index 174d143a6..ee99e9d0f 100644
--- a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
+++ b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
@@ -158,25 +158,25 @@ public void watchConfigKeyValues() {
new QueryParams(this.properties.getWatch().getWaitTime(), currentIndex));
// if response.value == null, response was a 404, a key is deleted
- Long newIndex = response.getConsulIndex();
- if (newIndex != null && !newIndex.equals(currentIndex)) {
- // don't publish the same index again, don't publish the first
- // time (-1) so index can be primed
- if (!this.consulIndexes.containsValue(newIndex) && !currentIndex.equals(-1L)) {
- if (log.isTraceEnabled()) {
- log.trace("Context " + context + " has new index " + newIndex);
- }
- RefreshEventData data = new RefreshEventData(context, currentIndex, newIndex);
- this.publisher.publishEvent(new RefreshEvent(this, data, data.toString()));
- }
- else if (log.isTraceEnabled()) {
- log.trace("Event for index already published for context " + context);
- }
- this.consulIndexes.put(context, newIndex);
- }
- else if (log.isTraceEnabled()) {
- log.trace("Same index for context " + context);
- }
+ Long newIndex = response.getConsulIndex();
+ if (newIndex != null && !newIndex.equals(currentIndex)) {
+ // don't publish the same index again, don't publish the first
+ // time (-1) so index can be primed
+ if (!this.consulIndexes.containsValue(newIndex) && !currentIndex.equals(-1L)) {
+ if (log.isTraceEnabled()) {
+ log.trace("Context " + context + " has new index " + newIndex);
+ }
+ RefreshEventData data = new RefreshEventData(context, currentIndex, newIndex);
+ this.publisher.publishEvent(new RefreshEvent(this, data, data.toString()));
+ }
+ else if (log.isTraceEnabled()) {
+ log.trace("Event for index already published for context " + context);
+ }
+ this.consulIndexes.put(context, newIndex);
+ }
+ else if (log.isTraceEnabled()) {
+ log.trace("Same index for context " + context);
+ }
}
catch (Exception e) {
// only fail fast on the initial query, otherwise just log the error
diff --git a/spring-cloud-consul-core/src/test/java/org/springframework/cloud/consul/test/ConsulTestcontainers.java b/spring-cloud-consul-core/src/test/java/org/springframework/cloud/consul/test/ConsulTestcontainers.java
index e6adada6f..2402387bc 100644
--- a/spring-cloud-consul-core/src/test/java/org/springframework/cloud/consul/test/ConsulTestcontainers.java
+++ b/spring-cloud-consul-core/src/test/java/org/springframework/cloud/consul/test/ConsulTestcontainers.java
@@ -35,6 +35,9 @@ public class ConsulTestcontainers implements ApplicationContextInitializer<Confi
static final Logger logger = LoggerFactory.getLogger(ConsulTestcontainers.class);
+ /**
+ * the consul server
+ */
public static GenericContainer<?> consul = new GenericContainer<>("consul:1.7.2")
.withLogConsumer(new Slf4jLogConsumer(logger).withSeparateOutputStreams())
.waitingFor(Wait.forHttp("/v1/status/leader")).withExposedPorts(8500)
diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulHeartbeatTaskTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulHeartbeatTaskTests.java
index bda0b2cf7..deafc3279 100644
--- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulHeartbeatTaskTests.java
+++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/discovery/ConsulHeartbeatTaskTests.java
@@ -35,7 +35,7 @@
import static org.mockito.Mockito.verify;
/**
- * Test for ConsulHeartbeatTask
+ * Test for ConsulHeartbeatTask.
*
* @author Toshiaki Maki
*/
diff --git a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/serviceregistry/ConsulAutoServiceRegistrationRetryTests.java b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/serviceregistry/ConsulAutoServiceRegistrationRetryTests.java
index 55debe762..f000fcf5e 100644
--- a/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/serviceregistry/ConsulAutoServiceRegistrationRetryTests.java
+++ b/spring-cloud-consul-discovery/src/test/java/org/springframework/cloud/consul/serviceregistry/ConsulAutoServiceRegistrationRetryTests.java
@@ -47,9 +47,15 @@
@ContextConfiguration(initializers = ConsulTestcontainers.class)
public class ConsulAutoServiceRegistrationRetryTests {
+ /**
+ * the exception
+ */
@Rule
public ExpectedException exception = ExpectedException.none();
+ /**
+ * the output
+ */
@Rule
public OutputCaptureRule output = new OutputCaptureRule();
From 69664c12fa1fb30cf925e29f1a414fd56843c18a Mon Sep 17 00:00:00 2001
From: Daniel Wu <daniel.wu06@sap.com>
Date: Sat, 21 May 2022 12:21:21 +0800
Subject: [PATCH 3/5] fix_ut
---
.../springframework/cloud/consul/config/ConfigWatchTests.java | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
index 7b337ec36..18d6ab595 100644
--- a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
+++ b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
@@ -75,12 +75,12 @@ public void watchPublishesEvent() {
}
@Test
- public void watchWithNullValueDoesNotPublishEvent() {
+ public void watchForDeletedKeyPublishesEvent() {
ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class);
setupWatch(eventPublisher, null, "/app/");
- verify(eventPublisher, never()).publishEvent(any(RefreshEvent.class));
+ verify(eventPublisher, times(1)).publishEvent(any(RefreshEvent.class));
}
@Test
From 8e3a6ae1caec3c08eb1c9618dd3e924d48166b67 Mon Sep 17 00:00:00 2001
From: Daniel Wu <daniel.wu06@sap.com>
Date: Wed, 8 Jun 2022 15:04:15 +0800
Subject: [PATCH 4/5] fix checkstyle
---
.../springframework/cloud/consul/config/ConfigWatchTests.java | 1 -
1 file changed, 1 deletion(-)
diff --git a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
index 18d6ab595..9dd4ce594 100644
--- a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
+++ b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
@@ -38,7 +38,6 @@
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
From 0cdacf2b7f937ceaab4fffeb924e03058f43ecc1 Mon Sep 17 00:00:00 2001
From: Daniel Wu <daniel.wu06@sap.com>
Date: Wed, 8 Jun 2022 19:06:06 +0800
Subject: [PATCH 5/5] fix unit test on slow machines
---
.../cloud/consul/config/ConfigWatch.java | 5 ++---
.../cloud/consul/config/ConfigWatchTests.java | 17 +++++++++++------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
index ee99e9d0f..85309b339 100644
--- a/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
+++ b/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java
@@ -57,7 +57,7 @@ public class ConfigWatch implements ApplicationEventPublisherAware, SmartLifecyc
private final AtomicBoolean running = new AtomicBoolean(false);
- private LinkedHashMap<String, Long> consulIndexes;
+ private final LinkedHashMap<String, Long> consulIndexes;
private ApplicationEventPublisher publisher;
@@ -150,13 +150,12 @@ public void watchConfigKeyValues() {
// use the consul ACL token if found
String aclToken = this.properties.getAclToken();
- if (StringUtils.isEmpty(aclToken)) {
+ if (!StringUtils.hasLength(aclToken)) {
aclToken = null;
}
Response<List<GetValue>> response = this.consul.getKVValues(context, aclToken,
new QueryParams(this.properties.getWatch().getWaitTime(), currentIndex));
-
// if response.value == null, response was a 404, a key is deleted
Long newIndex = response.getConsulIndex();
if (newIndex != null && !newIndex.equals(currentIndex)) {
diff --git a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
index 9dd4ce594..36eb89b1e 100644
--- a/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
+++ b/spring-cloud-consul-config/src/test/java/org/springframework/cloud/consul/config/ConfigWatchTests.java
@@ -38,7 +38,7 @@
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.atLeastOnce;
import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.springframework.cloud.consul.config.ConsulConfigProperties.Format.FILES;
@@ -51,7 +51,7 @@ public class ConfigWatchTests {
private ConsulConfigProperties configProperties;
@Before
- public void setUp() throws Exception {
+ public void setUp() {
this.configProperties = new ConsulConfigProperties();
}
@@ -61,6 +61,11 @@ public void watchPublishesEventWithAcl() {
setupWatch(eventPublisher, new GetValue(), "/app/", "2ee647bd-bd69-4118-9f34-b9a6e9e60746");
+ // there are two threads to publish events here in this UT, the unit test main
+ // thread and the thread started by
+ // config.start(). If you set a breakpoint or have a slow machine, you will see
+ // the test cases fail if we
+ // times(1) here. To work around this problem we use atLeastOnce() here.
verify(eventPublisher, atLeastOnce()).publishEvent(any(RefreshEvent.class));
}
@@ -70,7 +75,7 @@ public void watchPublishesEvent() {
setupWatch(eventPublisher, new GetValue(), "/app/");
- verify(eventPublisher, times(1)).publishEvent(any(RefreshEvent.class));
+ verify(eventPublisher, atLeastOnce()).publishEvent(any(RefreshEvent.class));
}
@Test
@@ -79,7 +84,7 @@ public void watchForDeletedKeyPublishesEvent() {
setupWatch(eventPublisher, null, "/app/");
- verify(eventPublisher, times(1)).publishEvent(any(RefreshEvent.class));
+ verify(eventPublisher, atLeastOnce()).publishEvent(any(RefreshEvent.class));
}
@Test
@@ -134,11 +139,11 @@ public void firstCallDoesNotPublishEvent() {
Response<List<GetValue>> response = new Response<>(getValues, 1L, false, 1L);
when(consul.getKVValues(eq(context), anyString(), any(QueryParams.class))).thenReturn(response);
- ConfigWatch watch = new ConfigWatch(this.configProperties, consul, new LinkedHashMap<String, Long>());
+ ConfigWatch watch = new ConfigWatch(this.configProperties, consul, new LinkedHashMap<>());
watch.setApplicationEventPublisher(eventPublisher);
watch.watchConfigKeyValues();
- verify(eventPublisher, times(0)).publishEvent(any(RefreshEvent.class));
+ verify(eventPublisher, never()).publishEvent(any(RefreshEvent.class));
}
}