Allow configuring a timeout for `Endpoint` selection in `EndpointGroup` #4246

ikhoon · 2022-05-10T12:47:13Z

Motivation:

A DynamicEndpointGroup lazily resolves Endpoints even after a
request is started. Armeria clients use a connection timeout to wait for
the late Endpoints.

armeria/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java

Line 320 in cc1656e

    
           return endpointGroup.select(this, temporaryEventLoop, connectTimeoutMillis()).handle((e, cause) -> {

The default connection timeout is 3.2 seconds which might be enough for a
single EndpointGroup.

However, HealthCheckedEndpointGroup wraps an EndpointGroup that
could be a DynamicEndpointGroup as well. Let's say the delegate
EndpointGroup is a DNS EndpointGroup. If a request is started, a DNS
resolution and HTTP health check should be preceded. If a server
starts, it could take more resources and 3.2 seconds that can not make
sense to a complex EndpointGroup structure.

Modifications:

Add selectionTimeoutMillis() to EndpointGroup so that a
EndpointSelector uses the value to schedule a timeout.
Add selectionTimeout{Millis}(long) to
AbstractDynamicEndpointGroupBuilder in order to override the
timeout.
- Set 0 to the default selection timeout for static endpoints.
- Set the default connection timeout to the default selection timeout
  for DynamicEndpointGrouop.
- Set the default response timeout to the default selection timeout
  for HealthCheckedEndpointGroup, ZooKeeperEndpointGroup,
  EurekaEndpointGroup and ConsulDnsEndpointGroup.
- HealthCheckedEndpointGroup adds its selectionTimeout and
  delegate's selectionTimeout.
Raise EndpointSelectionTimeoutException if EndpointSelctor fails
to select an Endpoint within a timeout.
Make ZooKeeperEndpointGroupBuilder and EurekaEndpointGroupBuilder
implment DynamicEndpointGroupSetters.
Deprecate) AbstractEndpointSelector.select(ctx, executor, timeout)
in favor of AbstractEndpointSelector.select(ctx, executor)

Result:

You can now configure a timeout for an Endpoint selection of an
DynamicEndpointGroup.

DnsAddressEndpointGroup delegate = DnsAddressEndpointGroup.of(...);
HealthCheckedEndpointGroup healthGroup =
  HealthCheckedEndpointGroup
    .builder(delegate, "/health")
    .selectionTimeout(Duration.ofSeconds(10))  // 👈👈👈
    .build();

Motivation: A `DynamicEndpointGroup` lazily resolves `Endpoint`s even after a request is started. Armeria clients use a connection timeout to wait for the late `Endpoint`s. https://github.com/line/armeria/blob/cc1656e6c9f7ba6b04d115e0298e90b8cf7c0971/core/src/main/java/com/linecorp/armeria/client/DefaultClientRequestContext.java#L320 The default connection timeout is 3.2 seconds that might be enough for a single `EndpointGroup`. However, `HealthCheckedEndpointGroup` wraps an `EndpointGroup` that could be a `DynamicEndpointGroup` as well. Let's say the delegate `EndpointGroup` is a DNS `EndpointGroup`. If a request is started, a DNS resolution and HTTP health check should be preceded. If a server starts, it could take more resources and 3.2 seconds that can not make sense to a complex `EndpointGroup` structure. Modifications: - Add `selectionTimeoutMillis()` to `EndpointGroup` so that a `EndpointSelector` uses the value to schedule a timeout. - Add `selectionTimeout{Millis}(long)` to `AbstractDynamicEndpointGroupBuilder` in order to override the timeout. - Set 0 to the default selection timeout for static endpoints. - Set the default connection timeout to the default selection timeout for `DynamicEndpointGrouop`. - Set the default response timeout to the default selection timeout for `HealthCheckedEndpointGroup`, `ZooKeeperEndpointGroup`, `EurekaEndpointGroup` and `ConsulDnsEndpointGroup`. - `HealthCheckedEndpointGroup` adds its `selectionTimeout` and delegate's `selectionTimeout`. - Raise `EndpointSelectionTimeoutException` if `EndpointSelctor` fails to select an `Endpoint` within a timeout. - Make `ZooKeeperEndpointGroupBuilder` and `EurekaEndpointGroupBuilder` implment `DynamicEndpointGroupSetters`. - Deprecate) `AbstractEndpointSelector.select(ctx, executor, timeout)` in favor of `AbstractEndpointSelector.select(ctx, executor)` Result: - You can configure a timeout for an `Endpoint` selection of an `DynamicEndpointGroup`. ``` DnsAddressEndpointGroup delegate = DnsAddressEndpointGroup.of(...); HealthCheckedEndpointGroup healthGroup = HealthCheckedEndpointGroup .builder(delegate, "/health") .selectionTimeout(Duration.ofSeconds(10)) // 👈👈👈 .build(); ```

codecov · 2022-05-13T07:51:50Z

Codecov Report

Merging #4246 (eba5255) into master (6f256cb) will increase coverage by 0.18%.
The diff coverage is 80.86%.

@@             Coverage Diff              @@
##             master    #4246      +/-   ##
============================================
+ Coverage     73.27%   73.46%   +0.18%     
- Complexity    17253    17340      +87     
============================================
  Files          1473     1475       +2     
  Lines         64966    65093     +127     
  Branches       8158     8169      +11     
============================================
+ Hits          47606    47821     +215     
+ Misses        13218    13117     -101     
- Partials       4142     4155      +13

Impacted Files	Coverage Δ
...inecorp/armeria/client/endpoint/EndpointGroup.java	`81.25% <ø> (ø)`
...m/linecorp/armeria/internal/client/ClientUtil.java	`84.78% <ø> (ø)`
...a/client/endpoint/DynamicEndpointGroupBuilder.java	`36.36% <33.33%> (-6.50%)`	⬇️
.../internal/client/endpoint/StaticEndpointGroup.java	`84.61% <33.33%> (-6.69%)`	⬇️
...ain/java/com/linecorp/armeria/client/Endpoint.java	`91.28% <50.00%> (+0.54%)`	⬆️
...p/armeria/client/endpoint/OrElseEndpointGroup.java	`61.53% <50.00%> (+0.42%)`	⬆️
...eria/client/eureka/EurekaEndpointGroupBuilder.java	`35.78% <71.42%> (+5.30%)`	⬆️
...lient/zookeeper/ZooKeeperEndpointGroupBuilder.java	`70.96% <73.33%> (+0.37%)`	⬆️
.../endpoint/AbstractDynamicEndpointGroupBuilder.java	`81.25% <75.00%> (-18.75%)`	⬇️
...eck/AbstractHealthCheckedEndpointGroupBuilder.java	`83.09% <76.19%> (-3.18%)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f256cb...eba5255. Read the comment docs.

jrhee17

Looks mostly good! Left some minor questions

jrhee17 · 2022-05-17T01:17:41Z

.../src/main/java/com/linecorp/armeria/client/endpoint/AbstractDynamicEndpointGroupBuilder.java


    /**
     * Creates a new instance.
     */
-    protected AbstractDynamicEndpointGroupBuilder() {}
+    protected AbstractDynamicEndpointGroupBuilder() {
+        this(Flags.defaultConnectTimeoutMillis());


I think the default for DnsEndpointGroupBuilder will be connectTimeout instead of responseTimeout. I wanted to check if this is your intention.

If the intention is to set responseTimeout as the default for pre-defined EndpointGroups in armeria, what do you think of just removing the empty constructor?
We can catch these type of mistakes at compile-time this way.

Sorry for the late response.

I think the default for DnsEndpointGroupBuilder will be connectTimeout instead of responseTimeout. I wanted to check if this is your intention.

Currently, the default select timeout for DnsEndpointGroup is the default connection timeout.

armeria/core/src/test/java/com/linecorp/armeria/client/endpoint/SelectionTimeoutTest.java

Line 118 in af45d0b

assertSelectionTimeout(endpointGroup).isEqualTo(Flags.defaultConnectTimeoutMillis());

DNS servers usually return a response quickly because they cache DNS records. Therefore, 3.2 seconds may be reasonable.

By the way, I checked the default timeout for a DNS query. It is 5 seconds that can be a more sensible default timeout for `DnsEndpointGroup/

what do you think of just removing the empty constructor?

As this class is a public API, that could cause a breaking change.

As this class is a public API, that could cause a breaking change.

AbstractDynamicEndpointGroupBuilder is part of unstable APIs. Will remove the default constructor.

It sounds reasonable to use the same timeout for DnsEndpointGroup and our DNS resolver.

jrhee17 · 2022-05-17T09:11:15Z

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

+     *                               ...
+     *                               .build();
+     * assert endpointGroup.selectionTimeoutMillis() == 13000;
+     * }</pre>


nit; what do you think of adding an explanation that setting this will set both selectionTimeoutMillis and initialSelectionTimeoutMillis

trustin · 2022-05-31T12:21:26Z

.../src/main/java/com/linecorp/armeria/client/endpoint/AbstractDynamicEndpointGroupBuilder.java


    /**
-     * Creates a new instance.
+     * Creates a new instance with the default {@code selectionTimeoutMillis}.


Suggested change

* Creates a new instance with the default {@code selectionTimeoutMillis}.

* Creates a new instance with the specified default {@code selectionTimeoutMillis}.

.../src/main/java/com/linecorp/armeria/client/endpoint/AbstractDynamicEndpointGroupBuilder.java

core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java

trustin · 2022-05-31T12:27:14Z

core/src/main/java/com/linecorp/armeria/client/endpoint/EndpointGroup.java

@@ -143,6 +144,13 @@ static EndpointGroup of(EndpointSelectionStrategy selectionStrategy,
    @Override
    Endpoint selectNow(ClientRequestContext ctx);

+    /**
+     * Returns the timeout to wait until a successful {@link Endpoint} selection.
+     * {@code 0} means {@link #selectNow(ClientRequestContext)} should always return an {@link Endpoint}.


Could you also document what happens on the initial selection attempt? Will it wait indefinitely or just return null?

core/src/main/java/com/linecorp/armeria/client/endpoint/dns/DnsEndpointGroupBuilder.java

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

core/src/main/java/com/linecorp/armeria/internal/client/dns/DnsUtil.java

.../com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupBuilderTest.java

...corp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupSelectionTimeoutTest.java

.../linecorp/armeria/client/endpoint/healthcheck/AbstractHealthCheckedEndpointGroupBuilder.java

jrhee17

Looks good to me once @trustin 's comments are addressed 👍 Thanks @ikhoon ! 🙇 🚀 👍

minwoox

Thanks a lot!
LGTM once @trustin's comments are addressed. 😉

minwoox · 2022-06-13T08:15:11Z

core/src/main/java/com/linecorp/armeria/client/endpoint/AbstractEndpointSelector.java

@@ -73,25 +82,41 @@ public final CompletableFuture<Endpoint> select(ClientRequestContext ctx,
            return UnmodifiableFuture.completedFuture(endpoint);
        }

+        final long selectionTimeoutMillis = endpointGroup.selectionTimeoutMillis();
+        if (selectionTimeoutMillis == 0) {


nit: Would it be better to use -1 to indicate StaticEndpointGroup?

I don't think we need to differentiate the timeout of StaticEndpointGroup from others.
A DynamincEndpointGroup may have 0 selection timeout if it is made from a static resource such as a file or an environment variable.

…ion-timeout

core/src/main/java/com/linecorp/armeria/client/endpoint/EndpointGroup.java

trustin

Just one comment. Looks great otherwise!

trustin · 2022-07-01T14:52:58Z

core/src/main/java/com/linecorp/armeria/client/endpoint/DynamicEndpointGroupSetters.java

+     * If unspecified, {@link Flags#defaultConnectTimeoutMillis()} is used by default.
+     */
+    @UnstableApi
+    default DynamicEndpointGroupSetters selectionTimeout(Duration selectionTimeout) {


selectionTimeout or selectTimeout? I feel like selectionTimeout is correct, but we have connectTimeout 🤔

Probably selectionTimeout is better here because we have EndpointSelectionTimeoutException.

trustin

Nice work, @ikhoon 🙇

ikhoon added new feature deprecation labels May 10, 2022

ikhoon added this to the 1.17.0 milestone May 10, 2022

ikhoon requested review from jrhee17, minwoox and trustin as code owners May 10, 2022 12:47

ikhoon added 10 commits May 10, 2022 21:57

Update Javadoc

44b78e7

Remove cruft

262935e

Add initialSelectionTimeout option for HealthCheckedEndpointGroup

5837649

Fix a bug in a test

afb526d

Add UnstableApi and fix bugs

9030415

Merge branch 'master' into endpoint-selection-timeout

a34ce58

Fix a conflict

c399ece

Fix a flaky test

af45d0b

Merge branch 'master' into endpoint-selection-timeout

09afaef

polish

da308f5

jrhee17 reviewed May 17, 2022

View reviewed changes

Address comments by @jrhee

2a6d00b