You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2013/12/14 14:31:29 UTC
[jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Add OkHttp driver to support modern HTTP verbs.
Using this driver, all mock and live tests for the OpenStack Marconi API pass, including the ones that use PATCH requests with a body. However, the integration tests for the driver itself fail randomly.
An example test output can be found here: https://gist.github.com/nacx/7953970
DO NOT MERGE THIS PULL REQUEST! It is just made to help debugging the failing tests. Once all pass, this will be closed and a proper one will be made.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds okhttp
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/232
-- Commit Summary --
* Added OkHttp driver to support modern HTTP verbs
* Fixed README
-- File Changes --
M core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java (67)
A drivers/okhttp/README.txt (5)
A drivers/okhttp/pom.xml (75)
A drivers/okhttp/src/main/java/org/jclouds/http/okhttp/OkHttpCommandExecutorService.java (101)
A drivers/okhttp/src/main/java/org/jclouds/http/okhttp/config/OkHttpCommandExecutorServiceModule.java (43)
A drivers/okhttp/src/test/java/org/jclouds/http/okhttp/OkHttpCommandExecutorServiceTest.java (54)
A drivers/okhttp/src/test/resources/test.jks (0)
M drivers/pom.xml (1)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/232.patch
https://github.com/jclouds/jclouds/pull/232.diff
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
So, I think this PR is now ready to be reviewed and merged, after squashing the commits. @adriancole @demobox @everett-toews mind having a look at it?
There is only one thing I'm not sure how to manage: The `jclouds-okhttp` driver depends on `okhttp` 1.2.2-SNAPSHOT, as support for the PATCH method was introduced in that version but is still not released. How should we proceed with this SNAPSHOT dependency?
@abayer @demobox do you think it is acceptable to have the SNAPSHOT dependency while we are in 1.8.0-SNAPSHOT? If it is, when it comes to release 1.8.0, if 1.2.2 is still not released we should decide what to do, although we can just go ahead and wait until it's time to release 1.8.0 to discuss this.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30923588
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> +/**
> + * Base class for integration tests that use {@link MockWebServer} to verify the
> + * behavior of the HTTP workflows.
> + *
> + * @author Ignasi Barrera
> + */
> +public abstract class BaseMockWebServerTest {
> +
> + protected SSLContext sslContext;
> +
> + @BeforeClass(groups = "integration")
> + protected void setupSSL() {
> + try {
> + sslContext = new SslContextBuilder(InetAddress.getLocalHost().getHostName()).build();
> + } catch (GeneralSecurityException ex) {
> + throw Throwables.propagate(ex);
nit. faster to write `throw new RuntimeException(e);` :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624330
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #505](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/505/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32155297
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #487](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/487/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30918686
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #954](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/954/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31539765
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #497](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/497/) UNSTABLE
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/232#issuecomment-31731043
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #491](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/491/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31539748
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
OkHttp 1.3.0 has been released including PATCH support. I'll rebase & merge!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32155897
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> + * Creates a test api for the given class and URL.
> + */
> + protected <T extends Closeable> T api(Class<T> apiClass, String url) {
> + Properties properties = new Properties();
> + properties.setProperty(PROPERTY_TRUST_ALL_CERTS, "true");
> + properties.setProperty(PROPERTY_RELAX_HOSTNAME, "true");
> + addConnectionProperties(properties);
> + return ContextBuilder.newBuilder(AnonymousProviderMetadata.forApiOnEndpoint(apiClass, url))
> + .modules(ImmutableSet.<Module> of(createConnectionModule())).overrides(properties)
> + .buildApi(apiClass);
> + }
> +
> + /**
> + * Add the connection properties used to configure the tests.
> + */
> + protected abstract void addConnectionProperties(Properties props);
could be named to make clear these go into `ContextBuilder.overrides()`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624400
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by Ignasi Barrera <no...@github.com>.
>It is possible that okhttp is retrying the POST that is expected to fail
I did a quick test and run the tests with https://github.com/square/okhttp/pull/369 applied, and the failures were the same, so I think we can discard the retried POST thing. I'll continue refactoring the tests to use mockwebserver and see what we find out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30647516
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
Sure I'll do! The only major refactor is in the test classes, so I think it is safe to backport.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32190969
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #948](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/948/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30835200
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> + <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>okhttp</artifactId>
> + <version>${okhttp.version}</version>
> + </dependency>
> + <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <version>${okhttp.version}</version>
> + <scope>test</scope>
> + </dependency>
> + <dependency>
> + <groupId>org.eclipse.jetty</groupId>
> + <artifactId>jetty-security</artifactId>
> + <scope>test</scope>
> + </dependency>
Still need this one..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8640628
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> @@ -349,4 +349,4 @@ private void handleAction(HttpServletRequest request, HttpServletResponse respon
> }
> }
>
> -}
> +}
fuzz
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624291
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #949](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/949/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30849140
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> OkHttp 1.3.0 has been released including PATCH support.
Nice! Thanks, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32170483
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #969](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/969/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32155338
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> + // key manager)
> + client.setSslSocketFactory(sslContextSupplier.get().getSocketFactory());
> + } else if (utils.trustAllCerts()) {
> + client.setSslSocketFactory(untrustedSSLContextProvider.get().getSocketFactory());
> + }
> + }
> + return client.open(url);
> + }
> +
> + @Override
> + protected void configureRequestHeaders(HttpURLConnection connection, HttpRequest request) {
> + super.configureRequestHeaders(connection, request);
> + // OkHttp does not set the Accept header if not present in the request.
> + // Make sure we send a flexible one.
> + if (request.getFirstHeaderOrNull(HttpHeaders.ACCEPT) == null) {
> + connection.setRequestProperty(HttpHeaders.ACCEPT, "*/*");
You may also have something to say about http://apache.markmail.org/thread/uxqpukef4feeg232, although that discussion should probably continue on the dev@ list.
I see somebody's "OkHttp" filter was activated ;-) Happy New Year!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624767
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
Addressed last comments.
@demobox Before merging this, we should decide what to do with the SNAPSHOT dependency. We depend on 1.2.2-SNAPSHOT, as the support for the PATCH verb has been added there. Should we keep this PR open until 1.2.2 is released, or can we merge it, and see if it is released before we release 1.8?
@abayer WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31728303
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #933](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/933/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30579943
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by Adrian Cole <no...@github.com>.
OK, so the `BaseJettyTest` is written to redirect POST every 20 requests, and fail with 500 error every 10 requests that aren't redirected.
```java
} else if (request.getMethod().equals("POST")) {
// don't redirect large objects
if (request.getContentLength() < 10240 && redirectEveryTwentyRequests(request, response))
return;
if (failEveryTenRequests(request, response))
return;
```
Below, I'm saying I did stuff not as I hunted the code down, just lazy and figured it was probably me.
I wrote this to ensure we could follow redirects on POST as this could happen in amazon with virtual-hosted buckets. I also wanted to ensure users could silently recover from 500s on POST as AWS query apis had many safe commands that were tunneled over POST. Note this behavior has since screwed with rackspace in particular who have occasionally returned 500 *after* processing the POST!
It is probably more safe to retry a POST before it was fully sent (IOException) vs retrying based a server response code.
For the record, the only safe write is PUT (maybe PATCH in some cases): it may be entirely better to remove this contract or ensure it does *not* retry POST! This would imply adding logic at each api level to decide which commands are really safe even though they are presented as POST requests. Note that this issue isn't black/white, so you can also see the [issue on okhttp wrt silent retries](https://github.com/square/okhttp/issues/258) (and disabling them).
Now back to these tests. 3 tests failed, right? Notice all of them had `testPost` prefix. The tests are written to ensure that input streams are not retried (as this ensures input stream is not rebuffered), while buffered content is retried.
* `testPostAsInputStream` is written to fail on Exception vs retrying. This test checked to see if it failed at least one of out 5 requests and it didn't. Now, BaseJettyTest is written to fail one out of every 10 POSTs, so this could be more just bad luck. At any rate, this test could be rewritten to send a flag on POST which jetty will always fail on vs relying on random failures or redirect roulette.
* `testPostBinder` and `testPostContentLanguage` failed with an EOF error on `POST http://localhost:8124/`. The port 8124 hints that these failures were after a redirect. I'd focus on redirect.
If I had to rewrite these tests, which were crafted in 2009, today. I'd use MockWebServer and isolate only the behaviors we want, not relying on state like how many other tests may have POST'ed something, etc. At the cost of firing up one (or 2 in the case of redirects) MWS per test, flakiness like this could be immediately identifiable regardless of how they are run. Well worth a refactor imho. I'd probably also remove the logic to retry unsafe http methods on 500. You can look at [feign](https://github.com/Netflix/feign/blob/master/core/src/test/java/feign/FeignTest.java#L414) for some opinions of retries I've had recently, ex. retrying on failed ssl handshake.
Suspicions about these failures: It is possible that okhttp is retrying the POST that is expected to fail. It is possible that the mechanics of a redirect are misaligned. See if you can narrow down with MWS tests. Here's an [example of a 2 server test](https://github.com/Netflix/feign/blob/master/ribbon/src/test/java/feign/ribbon/LoadBalancingTargetTest.java#L40), if you want to isolate redirects. Caution.. these are guesses! The real problems could be elsewhere.
Anyway, I hope these hints help!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30581270
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by Ignasi Barrera <no...@github.com>.
A bit more info... The driver tests ALWAYS pass when executed from Eclipse but they fail randomly when run with Maven from the command line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30575952
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #713](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/713/) UNSTABLE
Looks like there's a problem with this pull request
[(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/232#issuecomment-31539682
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> + limitations under the License.
> +
> +-->
> +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
> + <modelVersion>4.0.0</modelVersion>
> + <parent>
> + <groupId>org.apache.jclouds</groupId>
> + <artifactId>jclouds-project</artifactId>
> + <version>1.8.0-SNAPSHOT</version>
> + <relativePath>../../project/pom.xml</relativePath>
> + </parent>
> + <groupId>org.apache.jclouds.driver</groupId>
> + <artifactId>jclouds-okhttp</artifactId>
> + <name>jclouds OkHttp Driver</name>
> + <packaging>bundle</packaging>
> + <description>OkHttp Driver</description>
"...Driver for Apache jclouds"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8640613
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds #713 UNSTABLE
Spurious [test failure](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/org.apache.jclouds$jclouds-compute/713/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31541496
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> @@ -198,6 +198,10 @@ public String getResult() {
> @PUT
> @Path("/objects/{id}")
> void putNothing(@PathParam("id") String id);
> +
> + @POST
> + @Path("/objects/{id}")
> + void postNothing(@PathParam("id") String id);
postNada() :) jk
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624411
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Bayer <no...@github.com>.
I'm strongly opposed to merging this in so long as there's a SNAPSHOT dependency - there's no guarantee that SNAPSHOTs will stick around, not change drastically, etc...we could put it in labs on the master branch, I guess, since we're not at all likely to be releasing from there any time soon, but definitely it shouldn't go into the core repo or the 1.6.x or 1.7.x branches until it can depend on a released version.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31791054
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #469](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/469/) UNSTABLE
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/232#issuecomment-30574228
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> Should we keep this PR open until 1.2.2 is released, or can we merge it,
Do we know when 1.2.2 is planned? If we want this now, we could also put this in labs and then decide to move it to the main branch only once the SNAPSHOT dep is gone? But that might be a but much overhead if the wait isn't going to be that long in any case..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31729098
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #486](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/486/) UNSTABLE
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/232#issuecomment-30849033
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #485](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/485/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30834990
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #470](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/470/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30580034
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
Just left a comment in the OkHttp community. Let's see if we get an answer!
https://plus.google.com/103988187287212621453/posts/7u2dnYXhQnD
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31792290
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #950](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/950/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30918810
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by Ignasi Barrera <no...@github.com>.
This helps a lot. Thanks!
I'll refactor the POST tests to use mockwebserver, and try to isolate what is going on. I hope this will bring some light here!
I fully agree on the retry on 500 thing, but we'd better open an improvement for that. The code exists to accommodate real behaviors of existing APIs, and although I agree that should be moved to each concrete api and not be pre-configured in a generic way, I think it is better to properly scope that in its own JIRA.
Thanks again!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30594728
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
> + // key manager)
> + client.setSslSocketFactory(sslContextSupplier.get().getSocketFactory());
> + } else if (utils.trustAllCerts()) {
> + client.setSslSocketFactory(untrustedSSLContextProvider.get().getSocketFactory());
> + }
> + }
> + return client.open(url);
> + }
> +
> + @Override
> + protected void configureRequestHeaders(HttpURLConnection connection, HttpRequest request) {
> + super.configureRequestHeaders(connection, request);
> + // OkHttp does not set the Accept header if not present in the request.
> + // Make sure we send a flexible one.
> + if (request.getFirstHeaderOrNull(HttpHeaders.ACCEPT) == null) {
> + connection.setRequestProperty(HttpHeaders.ACCEPT, "*/*");
I think it is safe to have it here. The spec says that not sending the Accept header is equivalent to send it with an `*/*` value, so it shouldn't be a problem to set it if it hasn't been explicitly defined.
Keeping this would allow us to use this driver in other existing jclouds APIs that currently don't set the header and are now working just because the default JRE implementation adds it. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8631270
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
I agree. I'll ping the okhttp folks and see if we can have an estimated release date for 1.2.2 so we can properly plan how/when we can get this merged.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31791580
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #722](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/722/) 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/232#issuecomment-31730408
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #934](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/934/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30580426
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #471](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/471/) UNSTABLE
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/232#issuecomment-30580375
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #961](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/961/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-31731069
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> @@ -252,6 +252,11 @@
> <version>8.1.8.v20121106</version>
> </dependency>
> <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <version>1.2.1</version>
might want to introduce a property for okhttp version
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624497
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> + properties.setProperty(PROPERTY_RELAX_HOSTNAME, "true");
> + addConnectionProperties(properties);
> + return ContextBuilder.newBuilder(AnonymousProviderMetadata.forApiOnEndpoint(apiClass, url))
> + .modules(ImmutableSet.<Module> of(createConnectionModule())).overrides(properties)
> + .buildApi(apiClass);
> + }
> +
> + /**
> + * Add the connection properties used to configure the tests.
> + */
> + protected abstract void addConnectionProperties(Properties props);
> +
> + /**
> + * Return the connection module that provides the HTTP driver to use in the tests.
> + */
> + protected abstract Module createConnectionModule();
could default this to the standard one, or note in javadoc location of it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624367
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by Ignasi Barrera <no...@github.com>.
So finally, I've finished refactoring and rebasing everything to the latest version of master. Things to consider:
* I've rewritten the entire `BaseHttpCommandExecutorServiceIntegrationTest` class to use mockwebserver in all tests (and also added a few more). I've taken special care in keeping all tests and semantics provided by the `BaseJettytest` and properly modeling them in each test using mockwebserver.
* There is only one test I haven't kept: the one that verifies that arbitrary HTTP verbs can be used. Does it still make sense, now that we know that the JRE default implementation does not support them? Also, the GAE driver doesn't support that either, so I think that test should be implemented in each specific driver that supports arbitrary HTTP verbs. Sounds good?
* I've also rewritten the `BackoffLimitedRetryJavaTest` to use mockwebserver too.
With these two changes the indeterminism introduced by the `BaseJettyTest` is gone and everything is properly tested.
A few more notes:
* I've commented out the JVM memory configuration for the integration tests. It was only set for the integration tests, but not for unit or live tests. Does this make sense? (I've also sent a mail to the ML about this).
* I've properly configured the tests in the GAE. Basically removed all overridden methods and rely on the `@BeforeMethod` annotation to prepare the tests. There was no need o override them all.
* The `BaseJettyTest` class cannot be removed because there are classes in some providers that use it. Refactoring those tests could be done in a new PR.
@adriancole Many thanks for your feedback, it's been very instructive and I'm learning a lot with this refactor :) Could you kindly have a look at the `BaseHttpCommandExecutorServiceIntegrationTest` class, just to make sure I haven't missed any specific bit that should be tested?
@everett-toews If CI builds pass now, could you checkout the branch, and configure the `MarconiApiMetadata` to include the `OkHttpCommandExecutorServiceModule` module, and see if everything works, including the PATCH methods?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-30833189
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs. DO NOT
MERGE! (#232)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #932](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/932/) UNSTABLE
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/232#issuecomment-30574329
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Adrian Cole <no...@github.com>.
> + // key manager)
> + client.setSslSocketFactory(sslContextSupplier.get().getSocketFactory());
> + } else if (utils.trustAllCerts()) {
> + client.setSslSocketFactory(untrustedSSLContextProvider.get().getSocketFactory());
> + }
> + }
> + return client.open(url);
> + }
> +
> + @Override
> + protected void configureRequestHeaders(HttpURLConnection connection, HttpRequest request) {
> + super.configureRequestHeaders(connection, request);
> + // OkHttp does not set the Accept header if not present in the request.
> + // Make sure we send a flexible one.
> + if (request.getFirstHeaderOrNull(HttpHeaders.ACCEPT) == null) {
> + connection.setRequestProperty(HttpHeaders.ACCEPT, "*/*");
nit. probably better to be explicit and add `@Accept` annotations on methods that need it. HttpUrlConnection does a lot of sneaky stuff :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8624471
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Ignasi Barrera <no...@github.com>.
And [merged](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commit;h=09a430744a47b9db874b9d9cc1e0903729e068f2)!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232#issuecomment-32156639
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> @@ -0,0 +1,16 @@
> +jclouds OkHttp driver
> +=====================
> +
> +A driver to use the OkHttp (http://square.github.io/okhttp/) client as an HTTP library in jclouds.
> +
> +This driver allows using modern HTTP verbs such as PATCH in the providers and APIs, and also
> +provider SPDY support.
"This driver adds support for use of modern HTTP verbs such as PATCH in providers and APIs, and also supports SPDY."?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8640600
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> @@ -198,6 +198,10 @@ public String getResult() {
> @PUT
> @Path("/objects/{id}")
> void putNothing(@PathParam("id") String id);
> +
> + @POST
> + @Path("/objects/{id}")
> + void postNothing(@PathParam("id") String id);
"enviarNada", no?;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8640571
Re: [jclouds] Add OkHttp driver to support modern HTTP verbs (#232)
Posted by Andrew Phillips <no...@github.com>.
> @@ -252,6 +252,11 @@
> <version>8.1.8.v20121106</version>
> </dependency>
> <dependency>
> + <groupId>com.squareup.okhttp</groupId>
> + <artifactId>mockwebserver</artifactId>
> + <version>1.2.1</version>
Already introduced in the driver POM, I see...perhaps simply move into the project POM?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/232/files#r8640654