Remove `EndpointGroupRegistry`, allow specifying `EndpointGroup` #2381

trustin · 2020-01-06T15:22:49Z

…when building a client.

Related: #1084

Motivation:

Using a 'string' as a way to specify a target endpoint group makes it hard to build a clear dependency between an EndpointGroup and a Client.

Modifications:

(Breaking) Remove EndpointGroupRegistry
Change the input parameter from Endpoint to EndpointGroup wherever applicable, to allow a user to specify an EndpointGroup when building a client, e.g. Clients.newClient(SessionProtocol, EndpointGroup).
ClientBuilderParams now provides all properties to required for creating a derived Client.
- (Breaking) ClientFactory.newClient(ClientBuilderParams) replaced all other newClient() methods.
  - A user was never supposed to use these methods but Clients.newClient() or Clients.builder() anyway.
- Add various public validation methods to ClientFactory, which is used by other ClientFactory implementations and DefaultClientBuilderParams.
(Breaking) Endpoint now always refers to a single host.
- 'group:groupName' notation is not used anymore.
- Remove Endpoint.ofGroup()
- Remove Endpoint.isGroup()
- Remove Endpoint.groupName()
- Remove Endpoint.resolve()
EndpointGroup is now an EndpointGroupSelector.
- (Breaking) ClientRequestContext.endpointSelector() has been replaced with endpointGroup().
- When creating most EndpointGroups, a user can specify an EndpointSelectionStrategy, which is weighted round-robin by default.
- Add EndpointGroup.selectionStrategy()
- Add EndpointGroup.of() which requires an EndpointSelectionStrategy
(Breaking) Overhaul armeria-retrofit
- Add ArmeriaRetrofit which provides the factory methods for Retrofit and ArmeriaRetrofitBuilder
- A user can now specify a WebClient instead of specifying ClientOptions.
- A user now has much more control over how a WebClient for a non-base URL is created.
Miscellaneous:
- Add Clients.isUndefinedUri()
- Add EmptyEndpointGroupException
- (Deprecation) EndpointSelectionStrategy.ROUND_ROBIN and WEIGHT_ROUND_ROBIN in favor or unweightedRoundRobin() and weightRoundRobin()
- (Deprecation) Some PropertiesEndpointGroup.of() in favor of PropertiesEndpointGroupBuilder.
- (Deprecation) ZooKeeperEndpointGroup constructors in favor of ZooKeeperEndpointGroup.of() and ZooKeeperEndpointGroupBuilder.
- (Breaking) StaticEndpointGroup is not public anymore.

Result:

Much cleaner API
No group name clashes
Lots of breaking changes if a user was using EndpointGroupRegistry
extensively.

trustin · 2020-01-06T15:24:11Z

Meant to create a draft PR but that big green button was so tempting. 🤣 Added [WIP] header.

…hen building a client. Motivation: See line#1084 Modifications: - Remove `EndpointGroupRegistry` - Make `Endpoint` always refer to a specific host - Change the input parameter `Endpoint` to `EndpointGroup` wherever possible, to allow a user to specify an `EndpointGroup` when building a client. - `EndpointGroup` now always has its own `EndpointGroupSelector`. - `ClientRequestContext.endpointSelector()` has been replaced with `endpointGroup()`. Result: - Much cleaner API - No group name clashes - Lots of breaking changes if a user was using `EndpointGroupRegistry` extensively. To-dos: - `ClientFactory` internally requires a URI even if an `EndpointGroup` is specified. We need to clean this up. - Retrofit `@Url` annotation support - Overall Javadoc clean-up, Checkstyle, ...

trustin · 2020-01-07T10:52:50Z

@anuraaga @minwoox @ikhoon How does it look? I think most stuff works, except armeria-retrofit which requires a base URL.

trustin · 2020-01-07T13:52:44Z

@kojilin Because this change removed EndpointGroupRegistry completely, armeria-retrofit required quite a bit of breaking changes as well. Here's the test case that shows the new API that allows:

using client-side load-balancing: https://github.com/trustin/armeria/blob/endpoint_group_meets_strategy/retrofit2/src/test/java/com/linecorp/armeria/client/retrofit2/ArmeriaCallFactoryTest.java#L447-L454
using non-default WebClient
- Enforcing H1C: https://github.com/trustin/armeria/blob/endpoint_group_meets_strategy/retrofit2/src/test/java/com/linecorp/armeria/client/retrofit2/ArmeriaCallFactoryTest.java#L520-L528
- Setting timeout: https://github.com/trustin/armeria/blob/endpoint_group_meets_strategy/retrofit2/src/test/java/com/linecorp/armeria/client/retrofit2/ArmeriaCallFactoryLargeStreamTest.java#L96-L105

What do you think? Is there any use case I did not cover yet? Do you need any convenience methods in ArmeriaRetrofitBuilder?

trustin · 2020-01-07T14:00:11Z

Build now passes with -PnoCheckstyle option. Will remove WIP after fixing Checkstyle violations and documentation.

ikhoon

How does it look?

I love this style that injects EndpointGroup to WebClient.builder(SessionProtocol, EndpointGroup) than using EndpointGroupRegistry.
Because it is clear and explicit. 👍

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

ikhoon · 2020-01-08T05:02:43Z

Oops... I tapped Approved. 😅

anuraaga

Thanks! This generally looks much better. Going to take a bit more to grok the retrofit part

retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/ArmeriaRetrofitBuilder.java

codecov · 2020-01-08T08:23:39Z

Codecov Report

Merging #2381 into master will decrease coverage by <.01%.
The diff coverage is 67.82%.

@@             Coverage Diff              @@
##             master    #2381      +/-   ##
============================================
- Coverage     73.49%   73.49%   -0.01%     
- Complexity    10682    10685       +3     
============================================
  Files           939      939              
  Lines         40930    40968      +38     
  Branches       5088     5093       +5     
============================================
+ Hits          30082    30108      +26     
- Misses         8246     8258      +12     
  Partials       2602     2602

Impacted Files	Coverage Δ	Complexity Δ
...orp/armeria/client/ClientRequestContextCaptor.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...java/com/linecorp/armeria/server/file/HttpVfs.java	`28.57% <ø> (ø)`	`2 <0> (ø)`	⬇️
.../linecorp/armeria/client/DefaultClientFactory.java	`75.86% <ø> (ø)`	`18 <0> (ø)`	⬇️
...n/java/com/linecorp/armeria/client/UserClient.java	`83.33% <ø> (ø)`	`13 <0> (ø)`	⬇️
...inecorp/armeria/client/endpoint/EndpointGroup.java	`75.67% <ø> (ø)`	`17 <0> (ø)`	⬇️
...m/linecorp/armeria/client/ClientBuilderParams.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...java/com/linecorp/armeria/server/RouteBuilder.java	`79.66% <ø> (ø)`	`51 <0> (ø)`	⬇️
...a/client/endpoint/EmptyEndpointGroupException.java	`83.33% <ø> (ø)`	`4 <0> (ø)`	⬇️
...ria/client/endpoint/EndpointSelectionStrategy.java	`100% <ø> (ø)`	`3 <0> (ø)`	⬇️
.../com/linecorp/armeria/client/WebClientBuilder.java	`65.45% <0%> (-12.81%)`	`6 <0> (ø)`
... and 31 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 8d0f543...919f008. Read the comment docs.

trustin · 2020-01-09T06:29:15Z

This pull request is now ready for full reviews.

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

anuraaga · 2020-01-09T06:36:24Z

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

    }

    private Scheme scheme() {
        return scheme == null ? Scheme.of(format, protocol) : scheme;
    }

-    private void ensureEndpoint() {
-        if (endpoint == null) {
+    private void ensureEndpointGroup() {


Maybe overkill but had a thought there is probably a compile-time way of ensuring this (UriClientBuilder vs EndpointClientBuilder interfaces)

Good point. We will also need the same for WebClientBuilder. Maybe it's too much? 🤔

So are we doing this or not? 😄

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

anuraaga · 2020-01-09T06:57:56Z

core/src/test/java/com/linecorp/armeria/client/HttpClientIntegrationTest.java

-            assertThat(res.status()).isEqualTo(HttpStatus.OK);
-            assertThat(res.contentUtf8()).isEqualTo("success");
-        } finally {
-            EndpointGroupRegistry.unregister(groupName);


Nice cleanup :)

anuraaga · 2020-01-09T07:09:33Z

retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/ArmeriaRetrofitBuilder.java

-    public ArmeriaRetrofitBuilder withClientOptions(
-            BiFunction<String, ? super ClientOptionsBuilder, ClientOptionsBuilder> configurator) {
-        this.configurator = requireNonNull(configurator, "configurator");
+    public ArmeriaRetrofitBuilder nonBaseClientFactory(


Since normal usage doesn't need to worry about this, I guess it's ok. But doesn't it seem like too advanced functionality? The asymmetry vs WebClient (the one I pointed to above) seems unnatural too - if we really want this functionality, we could move it to a ClientOption so it applies to both?

And while it doesn't actually solve the asymmetry problem by itself, I'm wondering whether we could just let the EndpointSelector layer take care of it, it seems simpler to reason about and don't even need to worry about a client cache

class HostAwareEndpointGroup implements EndpointGroup { public Endpoint select(ClientRequestContext ctx) { switch (ctx.request().authority()) { case "group-bar": return groupBarEndpointGroup.select(ctx); ...

I think ENDPOINT_REMAPPER option handles this better. What do you think?

retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/ArmeriaRetrofitBuilder.java

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

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

minwoox · 2020-01-08T09:18:58Z

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

    private final HostType hostType;
    @Nullable
    private String authority;

-    private Endpoint(String groupName) {


…HttpUrl`

anuraaga · 2020-01-10T05:48:59Z

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

+public final class WebClientOptions {
+
+    /**
+     * A {@link Function} that remaps an {@link Endpoint} or an absolute URL's authority into


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

minwoox · 2020-01-10T08:19:08Z

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

    }

    private Scheme scheme() {
        return scheme == null ? Scheme.of(format, protocol) : scheme;
    }

-    private void ensureEndpoint() {
-        if (endpoint == null) {
+    private void ensureEndpointGroup() {


So are we doing this or not? 😄

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

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

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

minwoox · 2020-01-10T08:48:36Z

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

-     */
-    <T> T newClient(Scheme scheme, Endpoint endpoint, @Nullable String path, Class<T> clientType,
-                    ClientOptions options);
+    Object newClient(ClientBuilderParams params);


Can ClientBuilderParams have a type parameter for the client type so that wrong ClientBuilderParams cannot be passed as a parameter? 🤔

I tried but it seems to make the overall type signature more complex without much gain, like adding another type parameter to UserClient.

Perhaps it's not worth it given a user is not supposed to create a client using a ClientBuilderParams?

Ah, thanks! It's fine as it is. 😄

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

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

trustin · 2020-01-11T03:20:49Z

Moved ENDPOINT_MAPPER to ClientOptions and it looks better. This will be useful to non-WebClients when:

A user is given with a client that talks to a single Endpoint and wants to override that.
We provide a way to send a request to an arbitrary Endpoint by using a single client instance in the future, e.g. THttpClient.execute("tbinary+http://foo.com/api", ...);

ikhoon · 2020-01-10T05:36:00Z

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

+    /**
+     * Returns the {@link String} that consists of path, query string and fragment.
+     */
+    String absolutePathRef(); // Name inspired by https://stackoverflow.com/a/47545070/55808


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

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

minwoox

Great job @trustin!

ikhoon

Awesome work! Thanks, @trustin

…e#2381) …when building a client. Related: line#1084 Motivation: Using a 'string' as a way to specify a target endpoint group makes it hard to build a clear dependency between an `EndpointGroup` and a `Client`. Modifications: - (Breaking) Remove `EndpointGroupRegistry` - Change the input parameter from `Endpoint` to `EndpointGroup` wherever applicable, to allow a user to specify an `EndpointGroup` when building a client, e.g. `Clients.newClient(SessionProtocol, EndpointGroup)`. - `ClientBuilderParams` now provides all properties to required for creating a derived `Client`. - (Breaking) `ClientFactory.newClient(ClientBuilderParams)` replaced all other `newClient()` methods. - A user was never supposed to use these methods but `Clients.newClient()` or `Clients.builder()` anyway. - Add various public validation methods to `ClientFactory`, which is used by other `ClientFactory` implementations and `DefaultClientBuilderParams`. - (Breaking) `Endpoint` now always refers to a single host. - 'group:groupName' notation is not used anymore. - Remove `Endpoint.ofGroup()` - Remove `Endpoint.isGroup()` - Remove `Endpoint.groupName()` - Remove `Endpoint.resolve()` - `EndpointGroup` is now an `EndpointGroupSelector`. - (Breaking) `ClientRequestContext.endpointSelector()` has been replaced with `endpointGroup()`. - When creating most `EndpointGroup`s, a user can specify an `EndpointSelectionStrategy`, which is weighted round-robin by default. - Add `EndpointGroup.selectionStrategy()` - Add `EndpointGroup.of()` which requires an `EndpointSelectionStrategy` - (Breaking) Overhaul `armeria-retrofit` - Add `ArmeriaRetrofit` which provides the factory methods for `Retrofit` and `ArmeriaRetrofitBuilder` - A user can now specify a `WebClient` instead of specifying `ClientOption`s. - A user now has much more control over how a `WebClient` for a non-base URL is created. - Miscellaneous: - Add `Clients.isUndefinedUri()` - Add `EmptyEndpointGroupException` - (Deprecation) `EndpointSelectionStrategy.ROUND_ROBIN` and `WEIGHT_ROUND_ROBIN` in favor or `unweightedRoundRobin()` and `weightRoundRobin()` - (Deprecation) Some `PropertiesEndpointGroup.of()` in favor of `PropertiesEndpointGroupBuilder`. - (Deprecation) `ZooKeeperEndpointGroup` constructors in favor of `ZooKeeperEndpointGroup.of()` and `ZooKeeperEndpointGroupBuilder`. - (Breaking) `StaticEndpointGroup` is not public anymore. Result: - Much cleaner API - No group name clashes - Lots of breaking changes if a user was using `EndpointGroupRegistry` extensively.

trustin added cleanup breaking change deprecation labels Jan 6, 2020

trustin added this to the 0.98.0 milestone Jan 6, 2020

trustin requested review from anuraaga, ikhoon and minwoox January 6, 2020 15:22

trustin changed the title ~~Remove EndpointGroupRegistry and allow specifying EndpointGroup w…~~ [WIP] Remove EndpointGroupRegistry and allow specifying EndpointGroup w… Jan 6, 2020

trustin changed the title ~~[WIP] Remove EndpointGroupRegistry and allow specifying EndpointGroup w…~~ [WIP] Remove EndpointGroupRegistry and allow specifying EndpointGroup … Jan 6, 2020

trustin force-pushed the endpoint_group_meets_strategy branch from f5b3ab0 to 91ca122 Compare January 6, 2020 15:28

trustin added 3 commits January 7, 2020 12:04

Use static factory methods for EndpointSelectionStrategy / Update docs

875f242

Add more properties to ClientBuilderParams / Simplify ClientFactory

0b18b73

Merge branch 'master' into endpoint_group_meets_strategy

bf70322

trustin force-pushed the endpoint_group_meets_strategy branch from a258f8b to cc9b8c5 Compare January 7, 2020 10:50

Make armeria-retrofit work again.

e49fb07

trustin force-pushed the endpoint_group_meets_strategy branch from cc9b8c5 to e49fb07 Compare January 7, 2020 13:43

Fix large stream test

9e55aa3

ikhoon approved these changes Jan 8, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/ClientBuilder.java Outdated Show resolved Hide resolved

ikhoon self-requested a review January 8, 2020 05:01

Revert bad changes

e116a7c

anuraaga reviewed Jan 8, 2020

View reviewed changes

retrofit2/src/main/java/com/linecorp/armeria/client/retrofit2/ArmeriaRetrofitBuilder.java Outdated Show resolved Hide resolved

trustin added 2 commits January 8, 2020 16:43

Checkstyle / Javadoc

e81590e

Fix site error

8fdf2cf

Merge branch 'master' into endpoint_group_meets_strategy

eb4160f

trustin changed the title ~~[WIP] Remove EndpointGroupRegistry and allow specifying EndpointGroup …~~ Remove EndpointGroupRegistry and allow specifying EndpointGroup … Jan 9, 2020

Clean-up / EndpointGroup extends EndpointSelector

0e9410b

anuraaga reviewed Jan 9, 2020

View reviewed changes

Absolute path starting with '/'

af0d9ce

minwoox reviewed Jan 9, 2020

View reviewed changes

trustin added 7 commits January 9, 2020 17:56

Move endpoint remapping to WebClient / Pass Endpoint instead of `…

bcdd37e

…HttpUrl`

Address the comments from @minwoox

18652c2

Address more comments from @minwoox / more validation

9683e9e

Address more and more comments from @minwoox

324d6a1

Fix failures

d5b443d

Add delegate() again

3a7f541

Fix more failures

ecf5e41

anuraaga approved these changes Jan 10, 2020

View reviewed changes

minwoox reviewed Jan 10, 2020

View reviewed changes

trustin added 5 commits January 10, 2020 22:00

unweighted -> nil

71c0aa5

Address minor comments

f0af234

Clean-up

6ac785f

Apply endpointRemapper to all clients

5ba7625

Checkstyle

f3218ab

ikhoon reviewed Jan 13, 2020

View reviewed changes

Address the comments from @ikhoon

919f008

minwoox approved these changes Jan 13, 2020

View reviewed changes

ikhoon approved these changes Jan 13, 2020

View reviewed changes

trustin changed the title ~~Remove EndpointGroupRegistry and allow specifying EndpointGroup …~~ Remove EndpointGroupRegistry, allow specifying EndpointGroup Jan 13, 2020

trustin merged commit c714ec1 into line:master Jan 13, 2020

trustin deleted the endpoint_group_meets_strategy branch January 13, 2020 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `EndpointGroupRegistry`, allow specifying `EndpointGroup` #2381

Remove `EndpointGroupRegistry`, allow specifying `EndpointGroup` #2381

trustin commented Jan 6, 2020 •

edited

Loading

trustin commented Jan 6, 2020

trustin commented Jan 7, 2020

trustin commented Jan 7, 2020

trustin commented Jan 7, 2020

ikhoon left a comment

ikhoon commented Jan 8, 2020

anuraaga left a comment

codecov bot commented Jan 8, 2020 •

edited

Loading

trustin commented Jan 9, 2020

anuraaga Jan 9, 2020

trustin Jan 9, 2020

minwoox Jan 10, 2020

trustin Jan 10, 2020

anuraaga Jan 9, 2020

anuraaga Jan 9, 2020

trustin Jan 9, 2020 •

edited

Loading

minwoox Jan 8, 2020

anuraaga Jan 10, 2020

minwoox Jan 10, 2020

minwoox Jan 10, 2020

trustin Jan 10, 2020

minwoox Jan 13, 2020

trustin commented Jan 11, 2020 •

edited

Loading

ikhoon Jan 10, 2020

minwoox left a comment

ikhoon left a comment

Remove EndpointGroupRegistry, allow specifying EndpointGroup #2381

Remove EndpointGroupRegistry, allow specifying EndpointGroup #2381

Conversation

trustin commented Jan 6, 2020 • edited Loading

trustin commented Jan 6, 2020

trustin commented Jan 7, 2020

trustin commented Jan 7, 2020

trustin commented Jan 7, 2020

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon commented Jan 8, 2020

anuraaga left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2020 • edited Loading

Codecov Report

trustin commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Jan 11, 2020 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Remove `EndpointGroupRegistry`, allow specifying `EndpointGroup` #2381

Remove `EndpointGroupRegistry`, allow specifying `EndpointGroup` #2381

trustin commented Jan 6, 2020 •

edited

Loading

codecov bot commented Jan 8, 2020 •

edited

Loading

trustin Jan 9, 2020 •

edited

Loading

trustin commented Jan 11, 2020 •

edited

Loading