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

Jump to bottom

Issue #1350 - Dynamic selection of the transport to use based on ALPN on the client side. #3313

Merged
merged 3 commits into from Mar 19, 2019
Merged

Issue #1350 - Dynamic selection of the transport to use based on ALPN on the client side. #3313

merged 3 commits into from Mar 19, 2019

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Jan 31, 2019

Issue #1350.

Introduced HttpClientTransportDynamic to be able to switch transport dynamically.
Refactored other transports and HttpDestination, removing now unnecessary classes.

Introduced ProxyProtocolClientConnectionFactory and used it for testing.
Updated OSGi tests now that jetty-client depends on jetty-alpn-client.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

@sbordet sbordet requested review from joakime and gregw January 31, 2019 17:20
@sbordet
Copy link
Contributor Author

sbordet commented Jan 31, 2019

@gregw with respect to what we discussed, here is what I ended up doing.

WRT having one destination with multiple connection pools, that is a larger change that impacts for example HttpClient.maxConnectionsPerDestination - it would now be maxConnectionPerDestinationPerProtocol or something like that.
With a dynamic transport it is still a coarse settings (e.g. one destination is http/1.1 may need 200 connections per destination, another destination is h2 and may need less), but backwards compatible. It is still possible to change that setting on a per-destination basis.

WRT setting the ClientConnectionFactory on the destination, this is now possible at construction time via a Function that takes the default chain of ClientConnectionFactory and returns a possibly different chain.

@sbordet sbordet force-pushed the jetty-10.0.x-1350-dynamic_client_transport branch 3 times, most recently from a2ffd6f to e2e7f99 Compare February 12, 2019 17:26
@joakime
Copy link
Contributor

joakime commented Feb 13, 2019

This PR needs an update/merge .

@sbordet sbordet force-pushed the jetty-10.0.x-1350-dynamic_client_transport branch from e2e7f99 to cab0f2d Compare February 13, 2019 17:19
@gregw
Copy link
Contributor

gregw commented Mar 3, 2019

@sbordet Do we need to get this resolved before a 10.0 release?

@sbordet
Copy link
Contributor Author

sbordet commented Mar 3, 2019

@gregw yes

@sbordet
Issue #1350 - Dynamic selection of the transport to use based on ALPN…
aa211b0 
… on the client side.

Introduced `HttpClientTransportDynamic` to be able to switch transport dynamically.
Refactored other transports and HttpDestination, removing now unnecessary classes.

Introduced ProxyProtocolClientConnectionFactory and used it for testing.
Updated OSGi tests now that jetty-client depends on jetty-alpn-client.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet force-pushed the jetty-10.0.x-1350-dynamic_client_transport branch from cab0f2d to aa211b0 Compare March 6, 2019 09:35
joakime
joakime reviewed Mar 6, 2019
View reviewed changes
jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
@@ -1109,7 +1140,7 @@ protected HttpField getAcceptEncodingField()

protected String normalizeHost(String host)
{
if (host != null && host.matches("\\[.*\\]"))
if (host != null && host.matches("\\[.*]"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange edit. Is this supposed to strip out the host from an IPv6 address/host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, in regexp you have to escape the opening [, but not the closing ]. That snippet tests whether the host name is written using the URI IPv6 notation.

joakime
joakime approved these changes Mar 6, 2019
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

It's a big change, but I like it! 👍

gregw
gregw reviewed Mar 7, 2019
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

It's rather different than the last time I looked, so I think you need to walk me through it, although I'm seeing things that look good.

jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java Outdated
void setMaxRequestsPerConnection(int maxRequestsPerConnection);
}

public static class Info
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the class.... but not sure Info is the right name. Target??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose Key - it's the key we use to hold destinations in a Map.

jetty-client/src/main/java/org/eclipse/jetty/client/DuplexHttpDestination.java Show resolved Hide resolved
jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnectionFactory.java Show resolved Hide resolved
@sbordet
Issue #1350 - Dynamic selection of the transport to use based on ALPN…
29913f9 
… on the client side.

Added javadocs and small changes after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Issue #1350 - Dynamic selection of the transport to use based on ALPN…
c661788 
… on the client side.

Simplified ProxyProtocolClientConnectionFactory
to use Origin.Address rather than InetSocketAddress.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
gregw
gregw approved these changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Latest changes look good and naming is better.
Can't say I've completely understood the whole code, but I think it is definitely worth merging so we can work towards an alpha release.

@sbordet sbordet merged commit ec8a1bd into jetty-10.0.x Mar 19, 2019
@sbordet sbordet deleted the jetty-10.0.x-1350-dynamic_client_transport branch March 19, 2019 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw approved these changes

@joakime joakime joakime approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sbordet @joakime @gregw