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.

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

Ensuring that JerseyRequest scoped ClientConfig gets propagated to HttpUriRequest #939

Merged
merged 1 commit into from Mar 20, 2015

Conversation

Projects
None yet
5 participants
@velocipedist
Contributor

velocipedist commented Mar 12, 2015

Motivation

JerseyClient and the ApacheHttpClient are statically configured at creation time. Dropwizard configuration builders ensure those configs are in sync. However, WebTarget objects can have the configuration overridden per generated request. This information will be added to the JerseyRequest. Unfortunately, the current implementation of the DropwizardApacheConnector does not propagate this onto the RequestConfig in the created HttpUriRequest.

In DW 0.7.1 this was working thanks to this code: http://grepcode.com/file/repo1.maven.org/maven2/com.sun.jersey.contribs/jersey-apache-client4/1.18.1/com/sun/jersey/client/apache4/ApacheHttpClient4Handler.java#241

This is currently blocking the migration of https://github.com/yammer/tenacity project to DW 0.8.0, as we need this functionality to sync Hystrix timeouts with those of the HttpClient.

Solution

The solution is to update the DropwizardApacheConnector to check the JerseyRequest#getConfiguration for properties (present only if overridden) that should be propagated to the HttpUriRequest specific RequestConfig instance.

I didn't want to loose the default values for not overridden properties, so I wrapped the CloseableHttpClient in a delegating class that exposes the default RequestConfig to use as basis.

@joschi joschi added this to the 0.8.1 milestone Mar 12, 2015

velocipedist added a commit to yammer/tenacity that referenced this pull request Mar 12, 2015

Merge pull request #19 from yammer/dropwizard8_fix_dw_timeoutbug
verified that test failures caused by a dw 0.8.0 bug with a tested fix submitted here: dropwizard/dropwizard#939

cleaned up the tests, as with the new structure of jersey client code, we don't need that many
@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 13, 2015

Member

Thanks for this PR.
I will take a closer look at this in the weekend.

Member

arteam commented Mar 13, 2015

Thanks for this PR.
I will take a closer look at this in the weekend.

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 14, 2015

Member

Can we replace the hamcrest matchers with AssertJ assertions in ConfiguredCloseableHttpClientTest to be inline with the rest of the code?

Member

jplock commented Mar 14, 2015

Can we replace the hamcrest matchers with AssertJ assertions in ConfiguredCloseableHttpClientTest to be inline with the rest of the code?

@arteam

View changes

Show outdated Hide outdated ...nt/src/main/java/io/dropwizard/client/ConfiguredCloseableHttpClient.java
@arteam

View changes

Show outdated Hide outdated ...client/src/main/java/io/dropwizard/client/DropwizardApacheConnector.java
@arteam

View changes

Show outdated Hide outdated ...rc/test/java/io/dropwizard/client/ConfiguredCloseableHttpClientTest.java
@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 15, 2015

Member

@velocipedist. I've reviewed this PR and gave directions, where it, in my opinion, could be improved.
But motivation of this change seems convincing for me.

Also things to notice for you:

  • Please, rebase you PR against the current master ans squash the commits. Unfortunately, it's not merging now.
  • If possible, add an integration test for WebTarget configuration, so we can test that a new configuration is really applying to a request.
Member

arteam commented Mar 15, 2015

@velocipedist. I've reviewed this PR and gave directions, where it, in my opinion, could be improved.
But motivation of this change seems convincing for me.

Also things to notice for you:

  • Please, rebase you PR against the current master ans squash the commits. Unfortunately, it's not merging now.
  • If possible, add an integration test for WebTarget configuration, so we can test that a new configuration is really applying to a request.
@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 16, 2015

Contributor

I can add an integration test - we have one in our tenacity project, so it should be quick and easy enough

Contributor

velocipedist commented Mar 16, 2015

I can add an integration test - we have one in our tenacity project, so it should be quick and easy enough

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 16, 2015

Contributor

@arteam just wrote some behavioral integration tests for this. However, this forced me to put a test-scoped dependency on dropwizard-testing in dropwizard-client. I've noticed you've got other integration tests which avoid doing that, so am not sure whether this was done on purpose and my change will not cause objections.

Contributor

velocipedist commented Mar 16, 2015

@arteam just wrote some behavioral integration tests for this. However, this forced me to put a test-scoped dependency on dropwizard-testing in dropwizard-client. I've noticed you've got other integration tests which avoid doing that, so am not sure whether this was done on purpose and my change will not cause objections.

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 17, 2015

Contributor

@arteam @joschi @jplock

I've applied the review feedback. We now have a single commit that is ready to merge.

Contributor

velocipedist commented Mar 17, 2015

@arteam @joschi @jplock

I've applied the review feedback. We now have a single commit that is ready to merge.

@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
@arteam

View changes

Show outdated Hide outdated dropwizard-client/src/test/resources/yaml/dropwizardApacheConnectorTest.yml
@arteam

View changes

Show outdated Hide outdated ...nt/src/test/java/io/dropwizard/client/DropwizardApacheConnectorTest.java
public CloseableHttpClient getClient() {
return closeableHttpClient;
}
}

This comment has been minimized.

@arteam

arteam Mar 19, 2015

Member

Trivial nitpick.

Please, add a newline in the end of the added files. It'a standard for text files on *nix systems.
Reference

@arteam

arteam Mar 19, 2015

Member

Trivial nitpick.

Please, add a newline in the end of the added files. It'a standard for text files on *nix systems.
Reference

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 19, 2015

Member

@velocipedist Thanks! DropwizardApacheConnectorTest is a great piece of work.
Couple tweaks and this PR is ready to go.

Member

arteam commented Mar 19, 2015

@velocipedist Thanks! DropwizardApacheConnectorTest is a great piece of work.
Couple tweaks and this PR is ready to go.

Ensuring that JerseyRequest scoped ClientConfig gets propagated to Ht…
…tpUriRequest

**Motivation**

JerseyClient and the ApacheHttpClient are statically configured at creation time. Dropwizard configuration builders ensure those configs are in sync. However, WebTarget objects can have the configuration overridden per generated request. This information will be added to the JerseyRequest. Unfortunately, the current implementation of the DropwizardApacheConnector does not propagate this onto the RequestConfig in the created HttpUriRequest.

In DW 0.7.1 this was working thanks to this code: http://grepcode.com/file/repo1.maven.org/maven2/com.sun.jersey.contribs/jersey-apache-client4/1.18.1/com/sun/jersey/client/apache4/ApacheHttpClient4Handler.java#241

This is currently blocking the migration of https://github.com/yammer/tenacity project to DW 0.8.0, as we need this functionality to sync Hystrix timeouts with those of the HttpClient.

**Solution**

The solution is to update the DropwizardApacheConnector to check the JerseyRequest#getConfiguration for properties (present only if overridden) that should be propagated to the HttpUriRequest specific RequestConfig instance.

I didn't want to loose the default values for not overridden properties, so I wrapped the CloseableHttpClient in a DTO object that bundles together the constructed client and default config used. This detail is package protected (as per code review feedback) and only used internally to construct the `DropwizardApacheConnector`. Other clients still use the original API of the `HttpClientBuilder`.

**Testing**
I've added an integration test that behaviourally tests that the config overrides take effect. This forced me to put a dependency `dropwizard-testing` in test scope in the `dropwizard-client` artifact. I hope this is OK.

Conflicts:
	dropwizard-client/src/test/java/io/dropwizard/client/HttpClientBuilderTest.java
@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 20, 2015

Contributor

@arteam acted on your feedback, but for the change of the IP used in a test. Please see my detailed comment - curious to know your opinion.

Contributor

velocipedist commented Mar 20, 2015

@arteam acted on your feedback, but for the change of the IP used in a test. Please see my detailed comment - curious to know your opinion.

arteam added a commit that referenced this pull request Mar 20, 2015

Merge pull request #939 from velocipedist/master
Ensuring that JerseyRequest scoped ClientConfig gets propagated to HttpUriRequest

@arteam arteam merged commit 99629ea into dropwizard:master Mar 20, 2015

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 20, 2015

Member

@velocipedist Thanks for so much effort on this issue and excellent write-up!

Member

arteam commented Mar 20, 2015

@velocipedist Thanks for so much effort on this issue and excellent write-up!

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 21, 2015

Contributor

@arteam thanks for your help!

btw. are there any timelines set on dw 0.8.1 release?

Contributor

velocipedist commented Mar 21, 2015

@arteam thanks for your help!

btw. are there any timelines set on dw 0.8.1 release?

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 21, 2015

Member

Sorry, I can't give any strict ETA on 0.8.1.
It will depend on the type of release - feature or maintenance. We haven't decided yet.

Member

arteam commented Mar 21, 2015

Sorry, I can't give any strict ETA on 0.8.1.
It will depend on the type of release - feature or maintenance. We haven't decided yet.

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock Mar 22, 2015

Member

@arteam my vote would be a maintenance release as we've already fixed 6 issues in that version so far (it'd be great to get #949 resolved as well if you know a fix that for that one). I know this issue is blocking Yammer from updating https://github.com/yammer/tenacity to DW 0.8, which I'm using for a project, so I'd personally like to see us create a release sooner rather than later if possible.

Member

jplock commented Mar 22, 2015

@arteam my vote would be a maintenance release as we've already fixed 6 issues in that version so far (it'd be great to get #949 resolved as well if you know a fix that for that one). I know this issue is blocking Yammer from updating https://github.com/yammer/tenacity to DW 0.8, which I'm using for a project, so I'd personally like to see us create a release sooner rather than later if possible.

@velocipedist

This comment has been minimized.

Show comment
Hide comment
@velocipedist

velocipedist Mar 23, 2015

Contributor

@thesmith perhaps you've got the bandwidth to look at #949 ?

Contributor

velocipedist commented Mar 23, 2015

@thesmith perhaps you've got the bandwidth to look at #949 ?

@thesmith

This comment has been minimized.

Show comment
Hide comment
@thesmith

thesmith Apr 7, 2015

Contributor

Looks like @joschi got to it before I had a chance :)

Contributor

thesmith commented Apr 7, 2015

Looks like @joschi got to it before I had a chance :)

arteam added a commit that referenced this pull request Jun 26, 2015

Ensuring that request-scoped config gets propagated to HttpUriRequest
The change makes the config changes introduced via
`Invocation.Builder#property(propertyName, propertyValue)` effective.
We want to use this pattern because it is truely request creation scoped.
In contrast the current implementation is only effective if config changes
are being introduced via `WebTarget#property(propertyName, propertyValue)`
which unfortunatly creates a new copy of copy of
`org.glassfish.jersey.client.ClientRuntime` which effectively makes it
innefective for implementing dynamic configuration (we saw OutOfMemory
errors due to that).

Closes #939

arteam added a commit that referenced this pull request Jun 26, 2015

Ensuring that request-scoped config gets propagated to HttpUriRequest
The change makes the config changes introduced via
`Invocation.Builder#property(propertyName, propertyValue)` effective.
We want to use this pattern because it is truely request creation scoped.
In contrast the current implementation is only effective if config changes
are being introduced via `WebTarget#property(propertyName, propertyValue)`
which unfortunatly creates a new copy of copy of
`org.glassfish.jersey.client.ClientRuntime` which effectively makes it
innefective for implementing dynamic configuration (we saw OutOfMemory
errors due to that).

Original change can be founde here: #939

Closes #1137

jplock added a commit that referenced this pull request Jul 1, 2015

Ensuring that request-scoped config gets propagated to HttpUriRequest
The change makes the config changes introduced via
`Invocation.Builder#property(propertyName, propertyValue)` effective.
We want to use this pattern because it is truely request creation scoped.
In contrast the current implementation is only effective if config changes
are being introduced via `WebTarget#property(propertyName, propertyValue)`
which unfortunatly creates a new copy of copy of
`org.glassfish.jersey.client.ClientRuntime` which effectively makes it
innefective for implementing dynamic configuration (we saw OutOfMemory
errors due to that).

Original change can be founde here: #939

Closes #1137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment