Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop updating endpoints when HealthCheckedEndpointGroup is closing #6063

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Stop updating endpoints when HealthCheckedEndpointGroup is closing #6063

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f5154cd
Stop updating endpoints when `HealthCheckedEndpointGroup` is closing
ikhoon Jan 10, 2025
6a88be8
lint
ikhoon Jan 10, 2025
5fd2e9f
address comments by @minwoox
ikhoon Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions ...ain/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ private void setCandidates(List<Endpoint> endpoints) {
}
initialized = true;
destroyOldContexts(contextGroup);
setEndpoints(allHealthyEndpoints());
setEndpoints0(allHealthyEndpoints());
return null;
});
} finally {
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of not calling the updateHealth at all from the DefaultHealthCheckerContext?

private DefaultHealthCheckerContext newCheckerContext(Endpoint endpoint) {
    return new DefaultHealthCheckerContext(endpoint, port, protocol, clientOptions, retryBackoff,
                                           this::updateHealth, this);
}

DefaultHealthCheckerContext(Endpoint endpoint, int port, SessionProtocol protocol,
                            ClientOptions clientOptions, Backoff retryBackoff,
                            BiConsumer<Endpoint, Boolean> onUpdateHealth,
                            ListenableAsyncCloseable listenableAsyncCloseable) {
  ...
}

// Use listenableAsyncCloseable.isClosing( ) to check if the HealthCheckedEndpointGroup is closing or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that there's no heavy logic in the DefaultHealthCheckerContext.
So probably just check isClosing() in the updateHealth() method would be enough.

Copy link
Contributor Author

@ikhoon ikhoon Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateHealth() is not the only method that calls setEndpoint(). If a context is lazily initialized after HealthCheckedEndpointGroup is closed, empty endpoints will be set to the delegate. That situation will not happen normally though.

armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java

Line 197 in d7a4d29

setEndpoints(allHealthyEndpoints());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant was adding the logic that checks isClosing() instead of replacing the current location.
There is a logic that iterates the contextGroupChain while holding a lock in updateHealth() method and I thought we don't have to do this when isClosing() is true.

armeria/core/src/main/java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroup.java

Line 250 in 62da203

private DefaultHealthCheckerContext findContext(Endpoint endpoint) {

But, if it's a relatively light job you think, I'm fine with the current logic. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, fixed.

setEndpoints(allHealthyEndpoints());
setEndpoints0(allHealthyEndpoints());
}
}

private void setEndpoints0(List<Endpoint> endpoints) {
if (isClosing()) {
return;
}
setEndpoints(endpoints);
}

@Override
public long selectionTimeoutMillis() {
return initialized ? selectionTimeoutMillis : initialSelectionTimeoutMillis;
Expand Down
32 changes: 32 additions & 0 deletions ...java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,38 @@ void cacheReflectsAttributeChanges() throws InterruptedException {
}
}

@Test
void shouldStopUpdatingEndpointsWhenClosing() throws InterruptedException {
final AtomicInteger counter = new AtomicInteger();
final Function<? super HealthCheckerContext, ? extends AsyncCloseable> 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);

final 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() {}
Expand Down
Loading