improvement: Introduce new URIPool and URISelector interfaces. #341
Conversation
Refactors large bits of the internal httpclient.Client retry logic. Currently we don't store any state across the lifecycle of multiple requests. Instead the current retry logic relies on a RequestRetrier and a URI selection mechanism. However the interfaces between the two logic machines doesn't provide clear boundaries and also makes it difficult to use recovered response state across different requests. I propose introducing a two new interfaces -- URIPool and URISelector. The URIPool is an abstraction that tracks server side errors across all requests. It also is responsible for updating the list of available uris from the configuration refreshable. Additionally the URISelector is an abstraction that allow for implementing many types of scored request weighting algorithms. For example, least used connection, rendezvous routing, zone aware routing, ect. Lastly the RequestRetrier still determines at where requests in general should be retried and the associated backoff logic. Work here is still a WIP.
…or of client having round-robin uri selector.
dtrejod
force-pushed
the
dt/uri-retry-pool-selector
branch
from
b8e031c to
a060bc2
Compare
| @@ -65,7 +65,7 @@ func TestErrorDecoderMiddlewares(t *testing.T) { | |||
| assert.True(t, ok) | |||
| assert.Equal(t, 307, code) | |||
| location, ok := httpclient.LocationFromError(err) | |||
| assert.True(t, ok) | |||
| assert.False(t, ok) | |||
There was a problem hiding this comment.
Since we are now using http.Response.Location() in the error decoder middleware, this test return false since there is no valid location header returned in this test.
| location, err := resp.Location() | ||
| if err == nil { | ||
| unsafeParams["location"] = location.String() | ||
| } |
There was a problem hiding this comment.
Uses the http.Response.Location() method here allows us to forgo passing in the useBaseURIOnly in the doOnce() method since Location() builds the proper full URI based on the http.Response.Request and returned Location header.
…ing redirects. Uncomment relevant tests in stateful uri pool for failed uris.
…le scenario to hit.
…ellation error. Cleanup err handling in balanced selector. Fallback to response status code if cgr error isn't defined.
dtrejod
force-pushed
the
dt/uri-retry-pool-selector
branch
from
4528f68 to
5e2d60f
Compare
…ctDuration if one isn't defined.
dtrejod
force-pushed
the
dt/uri-retry-pool-selector
branch
from
dcba04f to
cdfd790
Compare
| @@ -164,7 +168,6 @@ func (c *clientImpl) doOnce( | |||
| transport := clientCopy.Transport // start with the client's transport configured with default middleware | |||
|
|
|||
| // must precede the error decoders to read the status code of the raw response. | |||
| transport = wrapTransport(transport, c.uriScorer.CurrentURIScoringMiddleware()) | |||
There was a problem hiding this comment.
The scoring URI middleware was moved into clientImpl.middleware instantiated when the client is built here:
conjure-go-runtime/conjure-go-client/httpclient/client_builder.go
Lines 160 to 161 in 6bf0ab9
| @@ -55,7 +55,8 @@ type clientImpl struct { | |||
| errorDecoderMiddleware Middleware | |||
| recoveryMiddleware Middleware | |||
|
|
|||
| uriScorer internal.RefreshableURIScoringMiddleware | |||
| uriPool internal.URIPool | |||
| uriSelector internal.URISelector | |||
There was a problem hiding this comment.
These interfaces exist for the lifetime of the client - we don't believe we need to manage state on a request-specific basis, so not optimizing the interface for that. This works well for state managed over the liftime of the client and URI strategies that are stateless (i.e. rendezvous hashing).
| // provided) to determine if the request should be attempted. If the returned value is true, the retrier will have | ||
| // waited the desired backoff interval before returning when applicable. If the previous response was a redirect, the | ||
| // retrier will also return the URL that should be used for the new next request. | ||
| func (r *RequestRetrier) Next(resp *http.Response, err error) (bool, *url.URL) { |
There was a problem hiding this comment.
It feels weird to me that the retrier is still responsible for URI selection. It would read more clearly to me if the URI redirection logic was implemented behind the URI interfaces used in the client instead.
| r.currentURI = nextURI | ||
| r.offset = nextURIOffset | ||
| // retry with backoff | ||
| return r.retrier.Next(), nil |
There was a problem hiding this comment.
nit: I would separate backoff behavior into a different interface given the weird way we use the underlying Retrier interface is not intuitive.
| @@ -55,7 +55,8 @@ type clientImpl struct { | |||
| errorDecoderMiddleware Middleware | |||
| recoveryMiddleware Middleware | |||
|
|
|||
| uriScorer internal.RefreshableURIScoringMiddleware | |||
| uriPool internal.URIPool | |||
There was a problem hiding this comment.
URIPool is new functionality. I think the behavior can be captured behind a URISelector implementation so prefer to just use the refreshable slice of URIs directly for now.
Before this PR
The components of the request retrier in CGR were tightly coupled and hard to reason about. Due to how requests are retried currently, we are unable to preserve server state across requests very easily.
After this PR
Refactors large bits of the internal
httpclient.Clientretry/uri selection logic.Instead the current retry logic relies on a
RequestRetriermiddleware anda separate URI selection mechanism. These existing interfaces limit
our ability to implement smarter client side load balancing algorithms.
I propose introducing a two new interfaces --
URIPoolandURISelector.The URIPool is an abstraction that tracks server side errors across all
requests. It also is responsible for updating the list of available uris
from the config refreshable. Additionally the
URISelectoris an abstraction that allow forimplementing many types of client side load balancing algorithms. For
example, least used connection, rendezvous routing, zone aware routing,
strict client side sharing, ect. These two new types will persist state across requests
by acting as middleware themselves so we can actually implement better request concurrent models
akin to what https://github.com/palantir/dialogue does today.
Lastly the
RequestRetrierstill determines when requests in generalshould be retried and the associated backoff logic.
In short, the idea overall becomes:
Work here is still a WIP.
==COMMIT_MSG==
Introduce new URIPool and URISelector interfaces.
==COMMIT_MSG==
Benchmarks
Possible downsides?
This change is