Issue #132 - ClientConnector abstraction. #3267
Conversation
Introduced ClientConnector and refactored HttpClient transports, removing duplicated code that was connect() to a remote host. Refactored also HTTP2Client to reference ClientConnector. Refactored tests accordingly to the changes introduced in the implementations. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| protected void doStart() throws Exception | ||
| { | ||
| if (executor == null) | ||
| setExecutor(new QueuedThreadPool()); |
There was a problem hiding this comment.
Please add a "client" name to this QTP for troubleshooting reasons.
Added name to default executor and scheduler after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Rewrote HttpClientTransportOverUnixSockets in light of ClientConnector. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Reverted refactoring of newConnection() to avoid to bind the class to a too specific abstract method. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
I've not really had time to look in detail, but a quick looks reveals nothing but minor questions.
However, I'm still not seeing exactly how you build up the dynamic capabilities of the client by adding transports that will then be reflected as available ConnectionFactories? Perhaps some examples of setting up strange configurations would help?
I'll look again tomorrow... but don't wait for me if you have a +1 already.
| getClientConnector().setSelectors(selectors); | ||
| } | ||
|
|
||
| public HttpClientTransportOverHTTP(ClientConnector connector) |
There was a problem hiding this comment.
shouldn't the ultimate constructor be HttpClientTransportOverHTTP(ClientConnector connector, int selectors)?
| private int inputBufferSize = 8192; | ||
| private List<String> protocols = Arrays.asList("h2", "h2-17", "h2-16", "h2-15", "h2-14"); | ||
| private List<String> protocols = List.of("h2"); |
| protected SslContextFactory newSslContextFactory() | ||
| { | ||
| SslContextFactory sslContextFactory = new SslContextFactory(false); | ||
| sslContextFactory.setEndpointIdentificationAlgorithm("HTTPS"); |
There was a problem hiding this comment.
Perhaps it is time to do this by default in new SslContextFactory?
| { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> context = (Map<String, Object>)attachment; | ||
| ClientConnectionFactory factory = (ClientConnectionFactory)context.get(CLIENT_CONNECTION_FACTORY_CONTEXT_KEY); |
There was a problem hiding this comment.
Why is the connection factory a function of context and not the Connector (like the server?)
There was a problem hiding this comment.
Oh I see... it comes from the destination!
Issue #132.
The main changes are a new
ClientConnectorclass and refactoredAbstractConnectorHttpClientTransport(which impacts bothHttpClientTransportOverHTTPandHttpClientTransportOverFCGI),HTTP2ClientandHttpClientTransportOverHTTP2.The rest is just small modifications and simplifications to make everything work as before.