Move Endpoint selection to the beginning of client execution #1917

trustin · 2019-07-19T10:27:07Z

Motivation:

ClientRequestContext.endpoint() currently return an Endpoint which
may be 1) a host:port endpoint or 2) a group endpoint. Therefore, a
client decorator does not know which host the request will be sent to
exactly; it is determined after all decorators are evaluated. This makes
it hard for client decorators to do the following:

Injecting a header value derived from the target host name.
Accounting for the number of requests sent to each host.

Modifications:

Split the initialization of DefaultClientRequestContext into two:
1. constructor and 2) init() method.
Determine the target host at the context initialization phase.
Make ClientRequestContext.endpoint() nullable, so that the
decorators can handle the case where endpoint selection has failed.
Add ClientRequestContext.endpointSelector().
Add test cases for HTTP, gRPC and Thrift.
Miscellaneous:
- Add EndpointGroup.empty()

Result:

Client decorators now knows the exact target host of the request.
We can make Endpoint always refer to a host:port, which will greatly
simplify both users' code and ours in the future.
We now have a proper place (DefaultClientRequestContext.init()) that
can be used for EventLoop allocation and connection pooling in the
future.

~~It currently has the following issues:~~

When EndpointSelector.select() raises an exception, the construction of ClientRequestContext fails, which means you have no access to its RequestLog. We could make ClientRequestContext available by deferring select() until ClientRequestContext is constructed, but the decorators will still not be able to access to the request.
- ~~The same problem will arise when we move the connection acquisition logic (connection pooling) to the beginning of request execution. The decorators will never see the connection failure.~~

codecov · 2019-07-19T10:55:41Z

Codecov Report

Merging #1917 into master will decrease coverage by 0.01%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##             master    #1917      +/-   ##
============================================
- Coverage     73.28%   73.26%   -0.02%     
- Complexity     8987     9014      +27     
============================================
  Files           794      798       +4     
  Lines         35189    35236      +47     
  Branches       4324     4333       +9     
============================================
+ Hits          25788    25816      +28     
- Misses         7193     7194       +1     
- Partials       2208     2226      +18

Impacted Files	Coverage Δ	Complexity Δ
.../linecorp/armeria/client/ClientRequestContext.java	`94.11% <ø> (ø)`	`8 <0> (ø)`	⬇️
...main/java/com/linecorp/armeria/client/Clients.java	`73.01% <ø> (ø)`	`23 <0> (ø)`	⬇️
...va/com/linecorp/armeria/common/RequestContext.java	`59.37% <ø> (ø)`	`23 <0> (ø)`	⬇️
.../linecorp/armeria/common/StringValueConverter.java	`26.19% <0%> (-2.02%)`	`9 <0> (ø)`
...rp/armeria/client/ClientRequestContextWrapper.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...com/linecorp/armeria/internal/grpc/GrpcStatus.java	`66.17% <0%> (-1.48%)`	`25 <0> (ø)`
...a/com/linecorp/armeria/common/HttpHeadersBase.java	`73.54% <0%> (ø)`	`150 <0> (ø)`	⬇️
...armeria/common/logback/RequestContextExporter.java	`64.57% <0%> (-0.59%)`	`54 <0> (ø)`
...ent/circuitbreaker/KeyedCircuitBreakerMapping.java	`84% <0%> (-16%)`	`5 <0> (ø)`
...m/linecorp/armeria/client/grpc/ArmeriaChannel.java	`97.5% <100%> (ø)`	`8 <0> (ø)`	⬇️
... and 40 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 3ba74c5...38b328e. Read the comment docs.

trustin · 2019-07-22T05:57:52Z

@anuraaga @ikhoon @minwoox Thoughts?

anuraaga · 2019-07-22T06:08:45Z

Does it work for the failure case to set the Endpoint of the request to a FailedEndpoint or something and continue to execute the request, throwing when about to connect to the FailedEndpoint?

trustin · 2019-07-22T06:35:39Z

Does it work for the failure case to set the Endpoint of the request to a FailedEndpoint or something and continue to execute the request, throwing when about to connect to the FailedEndpoint?

That sounds good to me. We could also do the same when we move the connection pooling to the beginning. Sounds good?

anuraaga · 2019-07-22T06:37:18Z

Yup :)

trustin · 2019-07-23T06:01:03Z

I found it's weird to use Endpoint to represent different states that are not very similar to each other - group, host, failure, so I just made ClientRequestContext.endpoint() nullable as advised by @minwoox. What do you think? If that's fine, let me add some test cases and remove WIP.

trustin · 2019-07-23T06:02:15Z

That being said, I'd eventually like to make Endpoint represent only a host:port, i.e. no group, because EndpointSelector can be used instead. (Not in this pull request but later.)

anuraaga · 2019-07-23T06:07:35Z

Yeah that makes sense

Motivation: `ClientRequestContext.endpoint()` currently return an `Endpoint` which may be 1) a host:port endpoint or 2) a group endpoint. Therefore, a client decorator does not know which host the request will be sent to exactly; it is determined after all decorators are evaluated. This makes it hard for client decorators to do the following: - Injecting a header value derived from the target host name. - Accounting for the number of requests sent to each host. Modifications: - Split the initialization of `DefaultClientRequestContext` into two: 1) constructor and 2) `init()` method. - Determine the target host at the context initialization phase. - Make `ClientRequestContext.endpoint()` nullable, so that the decorators can handle the case where endpoint selection has failed. - Add `ClientRequestContext.endpointSelector()`. - Add test cases for HTTP, gRPC and Thrift. - Miscellaneous: - Add `EndpointGroup.empty()`. Result: - Client decorators now knows the exact target host of the request. - We can make `Endpoint` always refer to a host:port, which will greatly simplify both users' code and ours in the future. - We now have a proper place (`DefaultClientRequestContext.init()`) that can be used for `EventLoop` allocation and connection pooling in the future.

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

core/src/main/java/com/linecorp/armeria/internal/ClientUtil.java

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

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

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

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

grpc/src/main/java/com/linecorp/armeria/client/grpc/ArmeriaClientCall.java

core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java

minwoox

Great job. 👍

logback/src/main/java/com/linecorp/armeria/common/logback/RequestContextExporter.java

ikhoon · 2019-07-25T03:16:50Z

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

+    /**
+     * Creates a new {@link ClientRequestContext} whose properties and {@link Attribute}s are copied from this
+     * {@link ClientRequestContext}, except having a different {@link Request} and its own {@link RequestLog}.
+     */


Thanks for adding missed documentation. 👍

core/src/main/java/com/linecorp/armeria/server/DefaultServiceRequestContext.java

...src/test/java/com/linecorp/armeria/client/circuitbreaker/KeyedCircuitBreakerMappingTest.java

trustin · 2019-07-25T04:09:49Z

@anuraaga I also hardened the thread safety of ctx.additional{Request,Response}{Headers,Trailers}() using CAS. Let me know if you have a concern about this, although I think it will not be that expensive with the level of contention we expect.

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

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

core/src/main/java/com/linecorp/armeria/client/retry/RetryingHttpClient.java

ikhoon

Thank you for awesome job!

Motivation: `ClientRequestContext.endpoint()` currently return an `Endpoint` which may be 1) a host:port endpoint or 2) a group endpoint. Therefore, a client decorator does not know which host the request will be sent to exactly; it is determined after all decorators are evaluated. This makes it hard for client decorators to do the following: - Injecting a header value derived from the target host name. - Accounting for the number of requests sent to each host. Modifications: - Split the initialization of `DefaultClientRequestContext` into two: 1) constructor and 2) `init()` method. - Determine the target host at the context initialization phase. - Make `ClientRequestContext.endpoint()` nullable, so that the decorators can handle the case where endpoint selection has failed. - Add `ClientRequestContext.endpointSelector()`. - Add test cases for HTTP, gRPC and Thrift. - Miscellaneous: - Add `EndpointGroup.empty()`. Result: - Client decorators now knows the exact target host of the request. - We can make `Endpoint` always refer to a host:port, which will greatly simplify both users' code and ours in the future. - We now have a proper place (`DefaultClientRequestContext.init()`) that can be used for `EventLoop` allocation and connection pooling in the future.

trustin added new feature breaking change labels Jul 19, 2019

trustin requested review from anuraaga, minwoox and ikhoon July 19, 2019 10:27

codefromthecrypt mentioned this pull request Jul 20, 2019

Migrates AWS Elasticsearch Storage to armeria/jackson openzipkin/zipkin-aws#135

Merged

trustin force-pushed the select_endpoint_early branch 5 times, most recently from 97d0d24 to a85dad1 Compare July 23, 2019 05:33

trustin force-pushed the select_endpoint_early branch 4 times, most recently from aab1c37 to 071c4c1 Compare July 24, 2019 03:22

trustin marked this pull request as ready for review July 24, 2019 03:24

trustin force-pushed the select_endpoint_early branch from 071c4c1 to 89528cb Compare July 24, 2019 05:27

minwoox reviewed Jul 24, 2019

View reviewed changes

anuraaga reviewed Jul 24, 2019

View reviewed changes

minwoox reviewed Jul 24, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/retry/RetryingClient.java Show resolved Hide resolved

Address the comments from @minwoox and @anuraaga

5c71561

trustin added 2 commits July 25, 2019 12:42

Thread safety as suggested by @minwoox

2c72d5b

Checkstyle

108583c

minwoox approved these changes Jul 25, 2019

View reviewed changes

ikhoon reviewed Jul 25, 2019

View reviewed changes

...src/test/java/com/linecorp/armeria/client/circuitbreaker/KeyedCircuitBreakerMappingTest.java Outdated Show resolved Hide resolved

Address the comment from @ikhoon

733bf95

anuraaga approved these changes Jul 25, 2019

View reviewed changes

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

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

trustin added 2 commits July 25, 2019 14:51

Fix a silly bug in removeAdditional*()

1fc3bb0

Address the comments from @anuraaga 2

78b267c

ikhoon reviewed Jul 25, 2019

View reviewed changes

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

trustin added 4 commits July 25, 2019 16:07

Address the comment from @ikhoon 2

a26deb0

Address the comments from @ikhoon 3

e786b3d

Fix indentation

d6d82f1

Fix checkstyle

38b328e

ikhoon approved these changes Jul 25, 2019

View reviewed changes

trustin merged commit ef0fec2 into line:master Jul 25, 2019

trustin deleted the select_endpoint_early branch July 25, 2019 09:08

trustin added this to the 0.89.0 milestone Jul 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Endpoint selection to the beginning of client execution #1917

Move Endpoint selection to the beginning of client execution #1917

trustin commented Jul 19, 2019 •

edited

codecov bot commented Jul 19, 2019 •

edited

trustin commented Jul 22, 2019

anuraaga commented Jul 22, 2019

trustin commented Jul 22, 2019

anuraaga commented Jul 22, 2019

trustin commented Jul 23, 2019

trustin commented Jul 23, 2019 •

edited

anuraaga commented Jul 23, 2019

minwoox left a comment

ikhoon Jul 25, 2019

trustin commented Jul 25, 2019

ikhoon left a comment

Move Endpoint selection to the beginning of client execution #1917

Move Endpoint selection to the beginning of client execution #1917

Conversation

trustin commented Jul 19, 2019 • edited

codecov bot commented Jul 19, 2019 • edited

Codecov Report

trustin commented Jul 22, 2019

anuraaga commented Jul 22, 2019

trustin commented Jul 22, 2019

anuraaga commented Jul 22, 2019

trustin commented Jul 23, 2019

trustin commented Jul 23, 2019 • edited

anuraaga commented Jul 23, 2019

minwoox left a comment

Choose a reason for hiding this comment

ikhoon Jul 25, 2019

Choose a reason for hiding this comment

trustin commented Jul 25, 2019

ikhoon left a comment

Choose a reason for hiding this comment

trustin commented Jul 19, 2019 •

edited

codecov bot commented Jul 19, 2019 •

edited

trustin commented Jul 23, 2019 •

edited