You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2014/11/27 01:43:48 UTC
[jclouds] Upgrade to OkHttp 2.1.0 (#617)
This upgrades OkHttp to 2.1.0 and refactors the OkHttp driver to use its native methods. This PR could supersede https://github.com/jclouds/jclouds/pull/544 and fix [JCLOUDS-744](https://issues.apache.org/jira/browse/JCLOUDS-744).
Note that the integration test suite in the OkHttp driver is the same than the one for the default HTTP driver, so the coverage is pretty good. I've also executed the OpenStack Marconi live tests (Marconi uses the OkHttp driver by default) and only one failed but it is unrelated to the change.
This is just an initial implementation of the driver using the 2.1.0 version and the OkHttp native API, that should put us in a better place to address the commented improvements to the SSL connections. The current implementation is based on the default HTTP driver, and there will be room for improvement. In any case, it's a nice starting point :)
/cc @adriancole
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds okhttp-2.1.0
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/617
-- Commit Summary --
* Upgrade to OkHttp 2.1.0
-- File Changes --
M apis/ec2/src/test/java/org/jclouds/ec2/internal/BaseEC2ApiMockTest.java (6)
M apis/openstack-keystone/src/test/java/org/jclouds/openstack/v2_0/internal/BaseOpenStackMockTest.java (2)
M apis/openstack-swift/src/test/java/org/jclouds/openstack/swift/v1/features/ObjectApiMockTest.java (9)
M apis/swift/src/test/java/org/jclouds/openstack/swift/blobstore/strategy/internal/SequentialMultipartUploadStrategyMockTest.java (2)
M core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java (2)
M core/src/test/java/org/jclouds/http/BaseHttpCommandExecutorServiceIntegrationTest.java (11)
M core/src/test/java/org/jclouds/http/BaseMockWebServerTest.java (18)
M drivers/okhttp/src/main/java/org/jclouds/http/okhttp/OkHttpCommandExecutorService.java (173)
M project/pom.xml (2)
M providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/internal/BaseAWSEC2ApiMockTest.java (9)
M providers/hpcloud-objectstorage/src/test/java/org/jclouds/hpcloud/objectstorage/internal/BaseHPCloudObjectStorageMockTest.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/617.patch
https://github.com/jclouds/jclouds/pull/617.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1421](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1421/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64794552
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Jake Wharton <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
Oh, derp. Thanks for the clarification!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21056620
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrew Phillips <no...@github.com>.
> @demobox can you kindly do that?
Done. I've switched the jclouds-pull-request and jclouds jobs over to Java 7.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64935781
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-7 #332](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-7/332/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64796722
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -29,18 +29,16 @@
> import javax.net.ssl.SSLContext;
>
> import org.jclouds.ContextBuilder;
> +import org.jclouds.http.config.JavaUrlHttpCommandExecutorServiceModule;
is this intentional?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973287
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
>
> /**
> * Implementation of the <code>HttpCommandExecutorService</code> that uses the
> * OkHttp client to support modern HTTP methods such as PATCH.
> */
> @Singleton
> -public class OkHttpCommandExecutorService extends JavaUrlHttpCommandExecutorService {
> +public class OkHttpCommandExecutorService extends BaseHttpCommandExecutorService<Request> {
nice. glad we aren't extending the java one anymore
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973301
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -97,6 +97,15 @@
> </execution>
> </executions>
> </plugin>
> + <plugin>
> + <groupId>org.codehaus.mojo</groupId>
> + <artifactId>animal-sniffer-maven-plugin</artifactId>
> + <configuration>
> + <!-- The filesystem provider only works with Java 7, and it is only
Nice catch! I'll change this too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004293
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> overrides.setProperty(Constants.PROPERTY_MAX_RETRIES, "1");
> return ContextBuilder.newBuilder(new AWSEC2ProviderMetadata())
> .credentials(ACCESS_KEY, SECRET_KEY)
> - .endpoint("http://localhost:" + regionToServers.get(DEFAULT_REGION).getPort())
> + .endpoint("http://" + defaultServer.getHostName() + ":" + defaultServer.getPort())
curious.. did you wildcard change these for a specific reason? the change here and other files seems more verbose.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973504
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1422](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1422/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64819204
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
@demobox TL;DR; it would only hurt us to not move to animal sniffer to enforce Java language level 6. The project already has too many ways to break, or slow us down, without adding any value.
Square and google open source projects use animal sniffer to watch out for api level concerns for libraries they want to work on JRE 6 or android.
https://github.com/google/guice/blob/master/pom.xml#L269
https://github.com/square/okhttp/blob/master/pom.xml#L206
I don't think requiring the project to be built with JDK 6 is a long-term win. We already hacking to allow filesystem to conditionally compile. Moreover, we've already documented this approach https://issues.apache.org/jira/browse/JCLOUDS-747 Also we've noted that checking signatures is something we haven't yet done, but should. http://wiki.apache.org/jclouds/Coding%20Standards
"Note that jclouds Maven configuration prevents incorrect use of newer language features but not newer APIs (yet). "
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64810125
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
Reopened #617.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#event-199498655
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-7 #330](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-7/330/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64782481
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrew Phillips <no...@github.com>.
@nacx Thanks for this! Just wanted to check, given what we've learned with Guava, that we actually _need_ this new version of OkHttp and we're not likely to force users into breaking upgrades.
>From the code changes I'm pretty sure we _do_ need it, but wanted to check to make sure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64731990
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> return ContextBuilder.newBuilder(new EC2ApiMetadata())
> .credentials(ACCESS_KEY, SECRET_KEY)
> - .endpoint("http://localhost:" + regionToServers.get(DEFAULT_REGION).getPort())
> + .endpoint("http://" + defaultServer.getHostName() + ":" + defaultServer.getPort())
careful with hostnames. this usually crashes on build servers. that's why I tend to use localhost directly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973251
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> OkHttpClient client = new OkHttpClient();
> - URL url = request.getEndpoint().toURL();
> - client.setProxy(proxyForURI.apply(request.getEndpoint()));
> - if (url.getProtocol().equalsIgnoreCase("https")) {
> + client.setConnectTimeout(utils.getConnectionTimeout(), TimeUnit.MILLISECONDS);
> + client.setReadTimeout(utils.getSocketOpenTimeout(), TimeUnit.MILLISECONDS);
> + // do not follow redirects since https redirects don't work properly
> + // ex. Caused by: java.io.IOException: HTTPS hostname wrong: should be
> + // <adriancole.s3int0.s3-external-3.amazonaws.com>
> + client.setFollowRedirects(false);
nit. you probably want to configure a global OkHttpClient and then do OkHttpClient requestScoped = global.clone();
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973431
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1960](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1960/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64792931
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
It's quite late here :) I'll read the rest of the comments tomorrow and address all nits!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64731899
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1419](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1419/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64782049
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
> return ContextBuilder.newBuilder(new EC2ApiMetadata())
> .credentials(ACCESS_KEY, SECRET_KEY)
> - .endpoint("http://localhost:" + regionToServers.get(DEFAULT_REGION).getPort())
> + .endpoint("http://" + defaultServer.getHostName() + ":" + defaultServer.getPort())
I've found that MWS 2.1.0 does not bind 127.0.0.1, but the public IP address, and that made the tests fail.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973303
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-7 #333](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-7/333/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64820516
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
IIRC all the discussion that ended up in the JVM version rollback started in [JCLOUDS-747](https://issues.apache.org/jira/browse/JCLOUDS-747). The main concern about requiring Java 7 to *run* (which is different than the one required to *compile*) jclouds was closing the door to support Android. If that is our target (which I believe it is), then this change is fine and will prevent us from introducing runtime incompatible changes. On the other hand, if we want jclouds to be *built* with java 6 (I don't think that provides us much value), then this change shouldn't be merged.
Regarding the Animal Sniffer plugin, it is being used in [OkHttp itself](https://github.com/square/okhttp/blob/master/pom.xml#L191-L210) and it is what they use to make sure they are runtime even when [requiring java 7](https://github.com/square/okhttp/blob/master/pom.xml#L36) to build the project, so I can think we can trust it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64808777
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
Thanks! Jobs for downstream repos should be updated to (all them use MWS), I'll have a check later today and update the remaining ones, if any.
We should see the builds getting back to normal.
Thanks again!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64945863
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
@nacx thanks tons for working on this. very close!
cc @andreaturli
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64731809
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1959](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1959/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64786639
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1420](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1420/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64783820
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1957](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1957/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64737323
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> OkHttpClient client = new OkHttpClient();
> - URL url = request.getEndpoint().toURL();
> - client.setProxy(proxyForURI.apply(request.getEndpoint()));
> - if (url.getProtocol().equalsIgnoreCase("https")) {
> + client.setConnectTimeout(utils.getConnectionTimeout(), TimeUnit.MILLISECONDS);
> + client.setReadTimeout(utils.getSocketOpenTimeout(), TimeUnit.MILLISECONDS);
> + // do not follow redirects since https redirects don't work properly
> + // ex. Caused by: java.io.IOException: HTTPS hostname wrong: should be
> + // <adriancole.s3int0.s3-external-3.amazonaws.com>
> + client.setFollowRedirects(false);
> + client.setProxy(proxyForURI.apply(request.uri()));
> +
> + if (request.isHttps()) {
> if (utils.relaxHostname()) {
> client.setHostnameVerifier(verifier);
> }
> if (sslContextSupplier != null) {
TODO: allow users to configure ConnectionSpec, and get out of this mess!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973368
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
>
> @Inject
> - public OkHttpCommandExecutorService(HttpUtils utils, ContentMetadataCodec contentMetadataCodec,
> + protected OkHttpCommandExecutorService(HttpUtils utils, ContentMetadataCodec contentMetadataCodec,
how about package private. and make this class final. plus mark all fields private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004336
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> DelegatingRetryHandler retryHandler, IOExceptionRetryHandler ioRetryHandler,
> DelegatingErrorHandler errorHandler, HttpWire wire, @Named("untrusted") HostnameVerifier verifier,
> - @Named("untrusted") Supplier<SSLContext> untrustedSSLContextProvider, Function<URI, Proxy> proxyForURI)
> - throws SecurityException, NoSuchFieldException {
> - super(utils, contentMetadataCodec, retryHandler, ioRetryHandler, errorHandler, wire, verifier,
> - untrustedSSLContextProvider, proxyForURI);
> + @Named("untrusted") Supplier<SSLContext> untrustedSSLContextProvider, Function<URI, Proxy> proxyForURI) {
> + super(utils, contentMetadataCodec, retryHandler, ioRetryHandler, errorHandler, wire);
> + this.untrustedSSLContextProvider = checkNotNull(untrustedSSLContextProvider, "untrustedSSLContextProvider");
> + this.verifier = checkNotNull(verifier, "verifier");
> + this.proxyForURI = checkNotNull(proxyForURI, "proxyForURI");
> + // OkHttp client can be safely used to perform different requests.
> + // If a request needs to customize it, it is safe to invoke its clone()
> + // method to get a client scoped to that request.
> + this.globalClient = createOkHttpClient();
Use dependency injection please!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004350
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrea Turli <no...@github.com>.
Thanks @nacx seems like it is just-in-time collaboration: it will be super useful for docker, I'm sure! Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64756725
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
>jclouds-pull-requests #1419 FAILURE
Expected failure we can now ignore. A JDK6 can't build the project but now the Animal Sniffer plugin makes sure our code is runtime compatible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64782320
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> return ContextBuilder.newBuilder(new EC2ApiMetadata())
> .credentials(ACCESS_KEY, SECRET_KEY)
> - .endpoint("http://localhost:" + regionToServers.get(DEFAULT_REGION).getPort())
> + .endpoint("http://" + defaultServer.getHostName() + ":" + defaultServer.getPort())
in that case, why not use `defaultServer.getUrl("")`? This is long-winded way of saying that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004216
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
>
> /**
> * Implementation of the <code>HttpCommandExecutorService</code> that uses the
> * OkHttp client to support modern HTTP methods such as PATCH.
> */
> @Singleton
> -public class OkHttpCommandExecutorService extends JavaUrlHttpCommandExecutorService {
> +public class OkHttpCommandExecutorService extends BaseHttpCommandExecutorService<Request> {
> +
> + public static final String DEFAULT_USER_AGENT = String.format("jclouds/%s java/%s", JcloudsVersion.get(),
> + System.getProperty("java.version"));
> +
> + protected final Supplier<SSLContext> untrustedSSLContextProvider;
> + protected final Function<URI, Proxy> proxyForURI;
> + protected final HostnameVerifier verifier;
> + @Inject(optional = true)
Sure! My plan is to have the driver working without changing the basic behaviour, and then address the SSL improvements in a separate PR. Sound good?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973444
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrew Phillips <no...@github.com>.
> This way we can get rid of the Java 6 builder, but still be confident that our code will run in a JRE 6
> without issues. @demobox Can we disable again the Java 6 build once this commit is in?
Can certainly be done, sure. But moving to a situation where we support Java 6 but cannot _build_ the project on Java 6 seems a pretty big step for me, and puts a lot of trust into Animal Sniffer. I don't know it well enough to make a judgement call on whether that is a smart move.
Does anyone have any more experience of using Animal Sniffer, or examples of other projects in a similar situation (i.e. support Java X but can't build on JDK X) and how they handle it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64806641
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> import com.squareup.okhttp.OkHttpClient;
> +import com.squareup.okhttp.Request;
> +import com.squareup.okhttp.RequestBody;
> +import com.squareup.okhttp.Response;
>
> /**
> * Implementation of the <code>HttpCommandExecutorService</code> that uses the
> * OkHttp client to support modern HTTP methods such as PATCH.
> */
> @Singleton
not necessary, as there's no shared state we aren't passed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004377
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -97,6 +97,15 @@
> </execution>
> </executions>
> </plugin>
> + <plugin>
> + <groupId>org.codehaus.mojo</groupId>
> + <artifactId>animal-sniffer-maven-plugin</artifactId>
> + <configuration>
> + <!-- The filesystem provider only works with Java 7, and it is only
how about just changing to java 7 signatures? That way, java 8 won't sneak in unintentionally.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004229
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
>
> /**
> * Implementation of the <code>HttpCommandExecutorService</code> that uses the
> * OkHttp client to support modern HTTP methods such as PATCH.
> */
> @Singleton
> -public class OkHttpCommandExecutorService extends JavaUrlHttpCommandExecutorService {
> +public class OkHttpCommandExecutorService extends BaseHttpCommandExecutorService<Request> {
> +
> + public static final String DEFAULT_USER_AGENT = String.format("jclouds/%s java/%s", JcloudsVersion.get(),
> + System.getProperty("java.version"));
> +
> + protected final Supplier<SSLContext> untrustedSSLContextProvider;
> + protected final Function<URI, Proxy> proxyForURI;
> + protected final HostnameVerifier verifier;
> + @Inject(optional = true)
ok I suppose since it previously inherited the old guy, that makes sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973560
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
A thing to note from the first build failure is that it seems that MWS 2.1.0 requires java 7. At least the MockResponse class seems not to be compatible.
Adrian, do you know if there is an artifact/alternative for java 6?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64733886
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
maybe TODO: initialize mock server in before/afterMethod :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973266
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> }
>
> - @Override
> - protected HttpURLConnection initConnection(HttpRequest request) throws IOException {
> + protected OkHttpClient createOkHttpClient() {
make this a provides method in the guice module.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004417
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> A thing to note from the first build failure is that it seems that MWS
> 2.1.0 requires java 7. At least the MockResponse class seems not to be
> compatible.
> Adrian, do you know if there is an artifact/alternative for java 6?
>
We should configure animal sniffer and get out of needing to literally use
JDK 6.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64735596
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
>jclouds-pull-requests-java-7 #332 SUCCESS
It is happy now! Comments are welcome :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64797076
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
@adriancole all comments should be addressed now. I've also rebased and squashed and properly separated the code in the proper commits.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64818843
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -29,18 +29,16 @@
> import javax.net.ssl.SSLContext;
>
> import org.jclouds.ContextBuilder;
> +import org.jclouds.http.config.JavaUrlHttpCommandExecutorServiceModule;
Nope. It has been added by Eclipse b/c it is referenced in a javadoc comment. Will remove it and change the comment to use the full class name.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973375
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1961](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1961/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64799915
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrew Phillips <no...@github.com>.
> I don't think requiring the project to be built with JDK 6 is a long-term win.
Sorry, wasn't trying to imply that this should be a goal. I just haven't myself worked much with _other_ ways of ensuring code is Java 6 compliant. If Animal Sniffer is used elsewhere successfully (thanks for those examples) and that works reliably, that'll work for me too!
+1
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64824665
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> import com.squareup.okhttp.OkHttpClient;
> +import com.squareup.okhttp.Request;
> +import com.squareup.okhttp.RequestBody;
> +import com.squareup.okhttp.Response;
>
there are more reasons than patch. I'd just delete this javadoc.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004392
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
I've addressed the comments. Here is the summary:
* Refactored the code to reuse the OkHttp client.
* I built all downstream projects and the following changes will be required to Glacier and GCE: https://github.com/nacx/jclouds-labs-aws/commit/e5dc6b5555e24f18dd1bd12e3e136cde4cb2d724 and https://github.com/nacx/jclouds-labs-google/commit/069eb64292e36805316b736b2f0912de63f0daf0. Once this is merged I'll raise the corresponding PRs to checking those commits.
* I fixed the OpenStack Keystone retry handler to not release the Payload. @adriancole I think in general we should avoid doing that in the RetryHandlers: after them, always the response parser, or the error handler will be executed, and they should be the ones to have the responsibility of releasing the payload; not the RetryHandler, which only makes the "retry decision". Do you agree to set this as a practice?
* I've configured the Animal Sniffer plugin to detect runtime incompatibilities with Java 6. This way we can get rid of the Java 6 builder, but still be confident that our code will run in a JRE 6 without issues. @demobox Can we disable again the Java 6 build once this commit is in? (/cc @andrewgaul @everett-toews)
Once this is OK I'll squash the commits and leave only two: one for the OkHttp changes, and one for the Animal Sniffer plugin.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64781974
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1962](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1962/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64822599
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
>@adriancole @nacx Sorry, I wasn't trying to imply that this should be a goal.
No need for apologies! I didn't want to suggest that. So we are aligned. I'll merge this, and then we should disable the Java 6 PR builders. @demobox can you kindly do that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64827731
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -580,6 +580,26 @@
> </executions>
> </plugin>
> <plugin>
> + <groupId>org.codehaus.mojo</groupId>
pull this into a separate commit, marked as JCLOUDS-747
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004459
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
Yes, the purpose is to be able to use the latest SSL features in OkHttp and we need 2.1.0 for that. However, take into account that OkHttp is *only* used in tests, by MWS. Only users using the HTTP driver will actually see the version increment. Sounds good?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64732227
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
SGTM. sweet dreams!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64731933
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Jake Wharton <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
Or use `MockWebServerRule`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20974231
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
> return ContextBuilder.newBuilder(new EC2ApiMetadata())
> .credentials(ACCESS_KEY, SECRET_KEY)
> - .endpoint("http://localhost:" + regionToServers.get(DEFAULT_REGION).getPort())
> + .endpoint("http://" + defaultServer.getHostName() + ":" + defaultServer.getPort())
Fine by me :) Will change.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21004268
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
>
> Actually, we should dictate that apis or users who muck with SSL have to
> use okhttp 2.1+.
>
> How to best communicate this kind of information and ensure everyone knows
> about it?
>
No non-labs api does at the moment. All we need to do is document during
release of jclouds 1.9 or whatever docker is in that it requires okhttp per
X, where X is a link that describes this policy.
Probably it needs to be in the configuration reference
http://jclouds.apache.org/reference/configuration/ basically stating that
you cannot change http drivers of apis that screw with TLS. It would also
show how to optionally use ConfigurationSpec to control TLS for any api
(ex. those frightened of the next POODLE go here).
Then, there would be a coding standard or practice on the wiki that says we
have one way of dealing with TLS, so please don't propagate alternatives.
Does this answer?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64733461
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1418](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1418/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64731580
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
>
> /**
> * Implementation of the <code>HttpCommandExecutorService</code> that uses the
> * OkHttp client to support modern HTTP methods such as PATCH.
> */
> @Singleton
> -public class OkHttpCommandExecutorService extends JavaUrlHttpCommandExecutorService {
> +public class OkHttpCommandExecutorService extends BaseHttpCommandExecutorService<Request> {
> +
> + public static final String DEFAULT_USER_AGENT = String.format("jclouds/%s java/%s", JcloudsVersion.get(),
> + System.getProperty("java.version"));
> +
> + protected final Supplier<SSLContext> untrustedSSLContextProvider;
> + protected final Function<URI, Proxy> proxyForURI;
> + protected final HostnameVerifier verifier;
> + @Inject(optional = true)
scary.. we should remove this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973311
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
okie last comments in for first wave.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64810973
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -134,6 +134,6 @@ private boolean shouldContinue(HttpCommand command, HttpResponse response) {
>
> protected abstract HttpResponse invoke(Q nativeRequest) throws IOException, InterruptedException;
>
> - protected abstract void cleanup(Q nativeResponse);
> + protected abstract void cleanup(Q nativeRequest);
thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973272
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
I think Adrien meant that jclouds uses TestNG for testing, not jUnit, and AFAIK MWS rules are for jUnit?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21056617
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
>
> Or use MockWebServerRule
>
Sadly, this is testng..
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20974689
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
Actually, we should dictate that apis or users who muck with SSL have to
use okhttp 2.1+. It has proven too unruly otherwise. This means docker and
azurecompute (until azurecompute switches to oauth). We should also dictate
those who cannot control the JVM in a way that makes them feel good about
SSL should use okhttp.
The reason is below. We *really* don't want to attempt to recreate a
portable version of this here.
https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareup/okhttp/ConnectionSpec.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64732504
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-7 #329](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-7/329/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64733700
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Jake Wharton <no...@github.com>.
> @@ -339,7 +339,14 @@ public void testCreateWithTimeout() throws Exception {
>
> fail("testReplaceTimeout test should have failed with an HttpResponseException.");
> } finally {
> - server.shutdown();
> + try {
> + server.shutdown();
Er, I don't understand. The rule is for testing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r21049594
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Ignasi Barrera <no...@github.com>.
The build failure was due to a new test added in the last commit in master. I've rebased the branch and fixed it. Hopefully the Jenkins Java 7 builder will be happy now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64794076
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Andrew Phillips <no...@github.com>.
> Sounds good?
Yes, thanks for explaining.
> Actually, we should dictate that apis or users who muck with SSL have to
use okhttp 2.1+.
How to best communicate this kind of information and ensure everyone knows about it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64732729
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests-java-7 #331](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests-java-7/331/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617#issuecomment-64784778
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
Posted by Adrian Cole <no...@github.com>.
> OkHttpClient client = new OkHttpClient();
> - URL url = request.getEndpoint().toURL();
> - client.setProxy(proxyForURI.apply(request.getEndpoint()));
> - if (url.getProtocol().equalsIgnoreCase("https")) {
> + client.setConnectTimeout(utils.getConnectionTimeout(), TimeUnit.MILLISECONDS);
> + client.setReadTimeout(utils.getSocketOpenTimeout(), TimeUnit.MILLISECONDS);
> + // do not follow redirects since https redirects don't work properly
> + // ex. Caused by: java.io.IOException: HTTPS hostname wrong: should be
> + // <adriancole.s3int0.s3-external-3.amazonaws.com>
> + client.setFollowRedirects(false);
> + client.setProxy(proxyForURI.apply(request.uri()));
> +
> + if (request.isHttps()) {
not sure we need to guard this.. we probably can configure this globally, as there's no request-scoped difference here. If the request isn't ssl, this config simply wouldn't be used. less if statements is better.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/617/files#r20973472