Allow users to supply a custom HttpClientBuilder and HttpClientConnectionManager #352

tjcelaya · 2017-10-04T01:29:55Z

Resolves #310.

Purpose

Accept an HttpClientBuilder and associated HttpClientConnectionManager (since the connection manager handles SSL configuration) and allow MantaConnectionFactory to work with both provided and internally-constructed versions of both.

Why create a new class?

~~These two objects are closely coupled and one has a lifecycle that needs to be managed. I'm open to switching to passing the builder and manager directly but this way allows us to avoid null checks.~~ Keeping the class for use in #357

tjcelaya · 2017-10-04T21:04:26Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/http/MantaConnectionFactory.java

-     */
-    ConfigContext getConfig() {
-        return config;
-    }


Removing unused method that was mistakenly included by #344

…g HttpClientBuilder and HttpClientConnectionManager using MantaConnectionFactoryConfigurator Tests for MantaConnectionFactory and MantaConnectionFactoryConfigurator Include a comment about why we don't use Mockito.verify() Split retry handler test case

dekobon

LGTM

cburroughs

I glanced through this and asked a few clarifying questions but will be out Monday and unable to respond in a timely manner.

I think the largest missing piece is guidance (in the form of CHANGELOG and/or javadoc) for why a user should (or should not!) use these new methods. I think we probably want some language affirming that the defaults are expected to work well for a wide variety of workloards. And as I understand the original request in #310 this is less able tuning for performance, and more about integration of threadpool, lifecycles, etc.

cburroughs · 2017-10-06T20:40:36Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java

-     * This method is typically used by unit tests to inject connection factory
-     * customizations so we can simulate different network events.
+     * Creates a new instance of the Manta client based on user-provided connection objects. This allows for a larger
+     * degree of customization at the cost of


Trailing off?

Must've been interrupted, thanks for catching that.

cburroughs · 2017-10-06T20:47:41Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/http/MantaConnectionFactory.java

-            new BasicHeader(HttpHeaders.ACCEPT, "application/json, */*")
-    );
+    public static final Collection<? extends Header> HEADERS =
+            Collections.unmodifiableList(


Is this needed for this change? It looks like a fine improvement but was unsure if it was demonstrating one of the "spooky" issues with all this being user configurable.

I'll change these back to private since the decision was never made and is better addressed in #357 but the background is I punted on the decision between:

document default headers and tell MantaConnectionFactoryConfigurator users to include these headers in their HttpClientBuilder configuration

add these headers to whatever default headers are provided by the user. This might require inspecting the HttpClientBuilder's internal state to determine if we've already added them in Lazily read ConfigContext in MantaClient #311?

add a setDefaultHeaders method to MantaConnectionFactoryConfigurator so users can tell us what headers they want to mix into the HttpClientBuilder (this is only really relevant to Lazy-loading configuration for MantaClient #357)

something else?

One thought is to change the HEADERS constant into an array of org.apache.http.Header[] objects and store it as a constant in MantaHttpRequestFactory or in StandardHttpHelper and add the headers to every HttpHead, HttpGet, etc. This would be a solution that would always make sure the right headers were there regardless of the HttpClientBuilder that was used.

That suggestion led me to discover default headers are implemented as just another HttpRequestInterceptor, definitely going to use MantaHttpRequestFactory for that and remove our call to HttpClientBuilder#setDefaultHeaders (in #357). Thanks!

cburroughs · 2017-10-06T20:52:35Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java

+                DefaultsConfigContext.DEFAULT_DISABLE_NATIVE_SIGNATURES)) {
+            builder.providerCode("stdlib");
+        }
+        final ThreadLocalSigner signer = new ThreadLocalSigner(builder);


This block looks the same as in the other constructor. Should it be DRYed out?

I probably wrote this block,but I admit I am having trouble recalling why DefaultsConfigContext needs to explicitly be referenced? We allow config contexts that don't start with the defaults?

I'll simplify the constructors.

Regarding config defaults: people could pass a StandardConfigContext which would throw NPEs if accessed directly. I've been fighting with the tradeoff between documenting defaults by directly referencing the DefaultsConfigContext and expressing the default behavior through for example structuring if clauses and using BooleanUtils.isTrue() for handling nulls. This small check would probably benefit from the latter since it's a simple boolean check though.

cburroughs · 2017-10-06T20:53:26Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/client/MantaClient.java

-    MantaClient(final ConfigContext config,
-                final KeyPair keyPair,
-                final MantaConnectionFactory connectionFactory,
-                final ThreadLocalSigner signer) {


Dropping the ability to pass in a signer is intentional, correct?

This constructor was only used in a single test which was has been disabled for a while (by being commented out in the testng.xml file) and needs to be revisted. That test case is documented in #354 and could be achieved with the MantaConnectionFactoryConfigurator and some simple mocks.

cburroughs · 2017-10-06T20:56:27Z

...a-client-unshaded/src/test/java/com/joyent/manta/client/MantaClientConnectionFailuresIT.java

+     *
+     * @throws IOException
+     */
+    @Test(enabled = false)


Is the intention here to fix this resolve this before merging?

No, this test has been disabled since December 2016 and has been documented in #354 to be addressed soon (but not here).

cburroughs · 2017-10-06T20:56:59Z

...ent-unshaded/src/test/java/com/joyent/manta/http/MantaConnectionFactoryConfiguratorTest.java

@@ -0,0 +1,33 @@
+package com.joyent.manta.http;


^^ License headers

cburroughs · 2017-10-06T20:57:19Z

java-manta-client-unshaded/src/test/java/com/joyent/manta/http/MantaConnectionFactoryTest.java

@@ -0,0 +1,197 @@
+package com.joyent.manta.http;


^^ License headers.

…t's no longer required

…ilder

…able to connectionFactoryConfigurator users

dkocher

I pulled this and have green integration tests in cyberduckapp.

dkocher · 2017-10-08T21:00:21Z

...-client-unshaded/src/main/java/com/joyent/manta/http/MantaConnectionFactoryConfigurator.java

+    /**
+     * An existing {@link HttpClientConnectionManager} to use with the {@link #httpClientBuilder}.
+     */
+    private final HttpClientConnectionManager connectionManager;


This parameter could be omitted and set to null in MantaConnectionFactory.

Just to be clear, you'd expect to call HttpClientBuilder#setConnnectionManager with your own HttpClientConnectionManager before passing it into MantaConnectionFactoryConfigurator? In retrospect it does seem unnecessary to hold onto the connection manager since we won't be terminating it. Thanks for the input!

dkocher · 2017-10-08T21:00:59Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/http/MantaConnectionFactory.java

-                            true);
-            this.httpClientBuilder.addInterceptorLast(authInterceptor);
+        if (connectionFactoryConfigurator != null) {
+            this.connectionManager = connectionFactoryConfigurator.getConnectionManager();


Set to null instead.

dkocher · 2017-10-08T21:01:42Z

java-manta-client-unshaded/src/main/java/com/joyent/manta/http/MantaConnectionFactory.java

@@ -116,38 +117,49 @@
    private final HttpClientConnectionManager connectionManager;

    /**
+     * Flag indicating if we created the {@link #connectionManager} ourselves and are responsible for shutting it down.
+     */
+    private final boolean connectionManagerShared;


Remove unused variable as null check will suffice.

…rator. Keep the configurator since it will be in charge of lazily-reloading objects in the next PR

tjcelaya self-assigned this Oct 4, 2017

tjcelaya requested review from cburroughs and dekobon October 4, 2017 20:51

tjcelaya commented Oct 4, 2017

View reviewed changes

tjcelaya closed this Oct 5, 2017

tjcelaya force-pushed the enhancement/310-supply-custom-HttpClientBuilder branch from 798eeb1 to ed31c89 Compare October 5, 2017 18:23

tjcelaya reopened this Oct 5, 2017

tjcelaya added 4 commits October 5, 2017 11:35

Resolve collision with ~/.ssh/id_rsa

519ca06

Break out config init

1ce0e3b

Close test factories

e0e4e66

Stop leaking ThreadLocalSigners

7938bef

tjcelaya force-pushed the enhancement/310-supply-custom-HttpClientBuilder branch from ffd9bd3 to 7938bef Compare October 5, 2017 20:06

dekobon approved these changes Oct 5, 2017

View reviewed changes

tjcelaya mentioned this pull request Oct 6, 2017

Lazy-loading configuration for MantaClient #357

Merged

cburroughs reviewed Oct 6, 2017

View reviewed changes

tjcelaya added 6 commits October 6, 2017 14:36

Checking my style

9c6fa8b

Revert MantaConnectionFactory#HEADERS change for now

451956f

Consolidate constructors

40b2c7d

Stop validating the nullness of connectionFactoryConfigurator since i…

d986791

…t's no longer required

Merge branch 'master' into enhancement/310-supply-custom-HttpClientBu…

74951d9

…ilder

Include a more helpful message about the world of possibilities avail…

520c938

…able to connectionFactoryConfigurator users

dkocher reviewed Oct 8, 2017

View reviewed changes

tjcelaya force-pushed the enhancement/310-supply-custom-HttpClientBuilder branch from f84a9a7 to fc2e08b Compare October 9, 2017 17:54

Remove HttpClientConnectionManager from MantaConnectionFactoryConfigu…

990a3b5

…rator. Keep the configurator since it will be in charge of lazily-reloading objects in the next PR

tjcelaya force-pushed the enhancement/310-supply-custom-HttpClientBuilder branch from fc2e08b to 990a3b5 Compare October 9, 2017 18:16

tjcelaya added 2 commits October 9, 2017 11:32

Changelog

cb04c1e

Documentation update

7bbb4cc

tjcelaya merged commit ae71cf3 into TritonDataCenter:master Oct 9, 2017

tjcelaya deleted the enhancement/310-supply-custom-HttpClientBuilder branch October 9, 2017 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to supply a custom HttpClientBuilder and HttpClientConnectionManager #352

Allow users to supply a custom HttpClientBuilder and HttpClientConnectionManager #352

tjcelaya commented Oct 4, 2017 •

edited

Loading

tjcelaya Oct 4, 2017

dekobon left a comment

cburroughs left a comment

cburroughs Oct 6, 2017

tjcelaya Oct 6, 2017

cburroughs Oct 6, 2017

tjcelaya Oct 6, 2017 •

edited

Loading

dekobon Oct 6, 2017

tjcelaya Oct 6, 2017 •

edited

Loading

cburroughs Oct 6, 2017

tjcelaya Oct 6, 2017 •

edited

Loading

cburroughs Oct 6, 2017

tjcelaya Oct 6, 2017

cburroughs Oct 6, 2017

tjcelaya Oct 6, 2017

cburroughs Oct 6, 2017

cburroughs Oct 6, 2017

dkocher left a comment

dkocher Oct 8, 2017

tjcelaya Oct 9, 2017

dkocher Oct 8, 2017

dkocher Oct 8, 2017

Allow users to supply a custom HttpClientBuilder and HttpClientConnectionManager #352

Allow users to supply a custom HttpClientBuilder and HttpClientConnectionManager #352

Conversation

tjcelaya commented Oct 4, 2017 • edited Loading

Purpose

Why create a new class?

Choose a reason for hiding this comment

dekobon left a comment

Choose a reason for hiding this comment

cburroughs left a 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

tjcelaya Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjcelaya Oct 6, 2017 • 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

dkocher left a 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

tjcelaya commented Oct 4, 2017 •

edited

Loading

tjcelaya Oct 6, 2017 •

edited

Loading

tjcelaya Oct 6, 2017 •

edited

Loading

tjcelaya Oct 6, 2017 •

edited

Loading