Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
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)
@@ -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)
@@ -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)
@@ -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)
@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)
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/e5dc6be24f18dd1bd12e3e136cde4cb2d724 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)
[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)
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)
[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)
[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)
[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)
[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)
[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)
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)
[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)
[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)
[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)
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)
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)
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)
@@ -97,6 +97,15 @@ /execution /executions /plugin + plugin +groupIdorg.codehaus.mojo/groupId +artifactIdanimal-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)
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)
@@ -97,6 +97,15 @@ /execution /executions /plugin + plugin +groupIdorg.codehaus.mojo/groupId +artifactIdanimal-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)
@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)
DelegatingRetryHandler retryHandler, IOExceptionRetryHandler
ioRetryHandler,
DelegatingErrorHandler errorHandler, HttpWire wire,
@Named(untrusted) HostnameVerifier verifier,
- @Named(untrusted) SupplierSSLContext
untrustedSSLContextProvider, FunctionURI, Proxy proxyForURI)
- throws SecurityException, NoSuchFieldException {
- super(utils, contentMetadataCodec, retryHandler, ioRetryHandler,
errorHandler, wire, verifier,
-untrustedSSLContextProvider, proxyForURI);
+ @Named(untrusted) SupplierSSLContext
untrustedSSLContextProvider, FunctionURI, 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)
import com.squareup.okhttp.OkHttpClient; +import com.squareup.okhttp.Request; +import com.squareup.okhttp.RequestBody; +import com.squareup.okhttp.Response; /** * Implementation of the codeHttpCommandExecutorService/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)
}
- @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)
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)
@@ -580,6 +580,26 @@ /executions /plugin plugin +groupIdorg.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)
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)
@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)
[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)
[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)
[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)
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)
@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
[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)
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)
@@ -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)
@@ -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)
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)
/**
* Implementation of the codeHttpCommandExecutorService/code that uses
the
* OkHttp client to support modern HTTP methods such as PATCH.
*/
@Singleton
-public class OkHttpCommandExecutorService extends
JavaUrlHttpCommandExecutorService {
+public class OkHttpCommandExecutorService extends
BaseHttpCommandExecutorServiceRequest {
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)
@@ -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)
/**
* Implementation of the codeHttpCommandExecutorService/code that uses
the
* OkHttp client to support modern HTTP methods such as PATCH.
*/
@Singleton
-public class OkHttpCommandExecutorService extends
JavaUrlHttpCommandExecutorService {
+public class OkHttpCommandExecutorService extends
BaseHttpCommandExecutorServiceRequest {
+
+ public static final String DEFAULT_USER_AGENT = String.format(jclouds/%s
java/%s, JcloudsVersion.get(),
+ System.getProperty(java.version));
+
+ protected final SupplierSSLContext untrustedSSLContextProvider;
+ protected final FunctionURI, 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)
@@ -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)
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)
[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)
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)
/**
* Implementation of the codeHttpCommandExecutorService/code that uses
the
* OkHttp client to support modern HTTP methods such as PATCH.
*/
@Singleton
-public class OkHttpCommandExecutorService extends
JavaUrlHttpCommandExecutorService {
+public class OkHttpCommandExecutorService extends
BaseHttpCommandExecutorServiceRequest {
+
+ public static final String DEFAULT_USER_AGENT = String.format(jclouds/%s
java/%s, JcloudsVersion.get(),
+ System.getProperty(java.version));
+
+ protected final SupplierSSLContext untrustedSSLContextProvider;
+ protected final FunctionURI, 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)
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
Re: [jclouds] Upgrade to OkHttp 2.1.0 (#617)
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)
@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)
/**
* Implementation of the codeHttpCommandExecutorService/code that uses
the
* OkHttp client to support modern HTTP methods such as PATCH.
*/
@Singleton
-public class OkHttpCommandExecutorService extends
JavaUrlHttpCommandExecutorService {
+public class OkHttpCommandExecutorService extends
BaseHttpCommandExecutorServiceRequest {
+
+ public static final String DEFAULT_USER_AGENT = String.format(jclouds/%s
java/%s, JcloudsVersion.get(),
+ System.getProperty(java.version));
+
+ protected final SupplierSSLContext untrustedSSLContextProvider;
+ protected final FunctionURI, 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)
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)
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)
@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)
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)
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)
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)
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)
[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)
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)
@@ -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)
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)
@@ -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)
[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)
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
