From f5154cd274647cbd677ed42a415dc52f640568e1 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 10 Jan 2025 17:51:29 +0900 Subject: [PATCH 1/3] Stop updating endpoints when `HealthCheckedEndpointGroup` is closing Motivation: When `HealthCheckedEndpointGroup` is closing, it clears the internal endpoints. https://github.com/line/armeria/blob/011741dc51cf328b78488655e78ceca1cdc0a1b3/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/DefaultHealthCheckerContext.java#L135 In fact, the closing process is not a problem. However, when the endpoints changes, the change is propagated to `EndpointSelector`. Some `EndpointSelector` implementations warn about the empty endpoints. https://github.com/line/armeria/blob/62da203bd9e11d669476e6999937352749cd7339/core/src/main/java/com/linecorp/armeria/client/endpoint/WeightedRoundRobinStrategy.java#L81 The warning is not a real problem but a false positive alarm. For better compatibility with `EndpointSelector` implementations, I propose to stop additional endpoints from being updated when `HealthCheckedEndpointGroup` is closed. As a result, `EndpointSelector` will not updated with empty endpoints. Modifications: - Stop updating endpoints when `HealthCheckedEndpointGroup` is closing Result: You no loger see a false positive warning log when `HealthCheckedEndpointGroup` is closing. --- .../HealthCheckedEndpointGroup.java | 11 +++++-- .../HealthCheckedEndpointGroupTest.java | 32 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java index 637da21fe30..1fff69616a5 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java @@ -194,7 +194,7 @@ private void setCandidates(List endpoints) { } initialized = true; destroyOldContexts(contextGroup); - setEndpoints(allHealthyEndpoints()); + setEndpoints0(allHealthyEndpoints()); return null; }); } finally { @@ -303,10 +303,17 @@ private void updateHealth(Endpoint endpoint, boolean health) { // Each new health status will be updated after initialization of the first context group. if (updated && initialized) { - setEndpoints(allHealthyEndpoints()); + setEndpoints0(allHealthyEndpoints()); } } + private void setEndpoints0(List endpoints) { + if (isClosing()) { + return; + } + setEndpoints(endpoints); + } + @Override public long selectionTimeoutMillis() { return initialized ? selectionTimeoutMillis : initialSelectionTimeoutMillis; diff --git a/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java b/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java index 2b0452c5434..e60e828a564 100644 --- a/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java @@ -602,6 +602,38 @@ void cacheReflectsAttributeChanges() throws InterruptedException { } } + @Test + void shouldStopUpdatingEndpointsWhenClosing() throws InterruptedException { + final AtomicInteger counter = new AtomicInteger(); + final Function checkFactory = ctx -> { + counter.incrementAndGet(); + ctx.updateHealth(HEALTHY, null, null, null); + return AsyncCloseableSupport.of(); + }; + + final Endpoint candidate1 = Endpoint.of("candidate1"); + final Endpoint candidate2 = Endpoint.of("candidate2"); + + final MockEndpointGroup delegate = new MockEndpointGroup(); + delegate.set(candidate1, candidate2, candidate2); + + HealthCheckedEndpointGroup endpointGroup = + new HealthCheckedEndpointGroup(delegate, true, + 10000, 10000, + SessionProtocol.HTTP, 80, + DEFAULT_HEALTH_CHECK_RETRY_BACKOFF, + ClientOptions.of(), checkFactory, + HealthCheckStrategy.all(), + DEFAULT_ENDPOINT_PREDICATE); + assertThat(counter.get()).isEqualTo(2); + final EndpointComparator comparator = new EndpointComparator(); + assertThat(endpointGroup.endpoints()).usingElementComparator(comparator) + .containsOnly(candidate1, candidate2); + endpointGroup.close(); + assertThat(endpointGroup.endpoints()).usingElementComparator(comparator) + .containsOnly(candidate1, candidate2); + } + static final class MockEndpointGroup extends DynamicEndpointGroup { MockEndpointGroup() {} From 6a88be81d680ebf9a9b4a440999037200ee7731b Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Fri, 10 Jan 2025 18:53:32 +0900 Subject: [PATCH 2/3] lint --- .../endpoint/healthcheck/HealthCheckedEndpointGroupTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java b/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java index e60e828a564..f6ee0f93b2f 100644 --- a/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java +++ b/core/src/test/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java @@ -617,7 +617,7 @@ void shouldStopUpdatingEndpointsWhenClosing() throws InterruptedException { final MockEndpointGroup delegate = new MockEndpointGroup(); delegate.set(candidate1, candidate2, candidate2); - HealthCheckedEndpointGroup endpointGroup = + final HealthCheckedEndpointGroup endpointGroup = new HealthCheckedEndpointGroup(delegate, true, 10000, 10000, SessionProtocol.HTTP, 80, From 5fd2e9f416d2c78b7b73dc220abd61ca9485d508 Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Tue, 14 Jan 2025 15:18:53 +0900 Subject: [PATCH 3/3] address comments by @minwoox --- .../endpoint/healthcheck/HealthCheckedEndpointGroup.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java index 1fff69616a5..ea56d7b719e 100644 --- a/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java +++ b/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java @@ -291,6 +291,12 @@ private void destroyOldContexts(HealthCheckContextGroup contextGroup) { } private void updateHealth(Endpoint endpoint, boolean health) { + if (isClosing()) { + logger.debug("HealthCheckedEndpointGroup is closed. Ignoring health update for: {}. (healthy: {})", + endpoint, health); + return; + } + final boolean updated; // A healthy endpoint should be a valid checker context. if (health && findContext(endpoint) != null) {