Adding ClientFactory abstraction to allow alternate JAXRS implementations to be used by jskjons · Pull Request #340 · spotify/docker-client

jskjons · 2016-02-04T20:12:06Z

We use RESTEasy in most of our applications/tooling and because the docker client currently uses the standard JAXRS client api it's hard to control which implementation gets used and it's impossible to customize the client if Resteasy gets picked. This abstractions allows us to just create RESTEasy clients and then only have 1 JAXRS implementation on our classpath. I've tested it with both Jersey and RESTEasy (not included) implementations and everything seems to work.

I also switched the JerseyClientFactory to directly use the JerseyClientBuilder so that it'll still use Jersey even if there's an alternate JAXRS implementation on the classpath.

codecov-io · 2016-02-05T17:24:48Z

Current coverage is 62.78% (diff: 75.78%)

Merging #340 into master will increase coverage by 16.44%

@@             master       #340   diff @@
==========================================
  Files           130         81     -49   
  Lines          4331       2940   -1391   
  Methods           0          0           
  Messages          0          0           
  Branches        635        403    -232   
==========================================
- Hits           2007       1846    -161   
+ Misses         2134       1094   -1040   
+ Partials        190          0    -190

Powered by Codecov. Last update cbdd93b...128c40f

jwgmeligmeyling · 2016-04-30T12:08:55Z

@@ -57,11 +57,7 @@
 import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
 import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
 import org.glassfish.hk2.api.MultiException;


It seems this code is still dependent on org.glassfish.hk2.api.MultiException and org.glassfish.jersey.internal.util.Base64. Are you sure that this client still works when glassfish has completely vanished from the classpath? And that the exception handling works well if glassfish is replaced with resteasy?

MultiException is in hk2-api which doesn't seem to conflict with RESTEasy anyways so I don't think I need to worry about getting rid of the reference, although explicitly adding the maven dependency rather than using it transitively would be good. I'll make that change.

Base64 I didn't notice and I assume would've failed if I tried to use it with registry authentication. Any objections to switching that to commons-codec?

We could also switch to Java8 and use Base64.getEncoder()

On Mon, 2 May 2016 at 18:19 Jeff Skjonsby notifications@github.com wrote:

In src/main/java/com/spotify/docker/client/DefaultDockerClient.java
#340 (comment):

@@ -57,11 +57,7 @@
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.glassfish.hk2.api.MultiException;

MultiException is in hk2-api which doesn't seem to conflict with RESTEasy
anyways so I don't think I need to worry about getting rid of the
reference, although explicitly adding the maven dependency rather than
using it transitively would be good. I'll make that change.

Base64 I didn't notice and I assume would've failed if I tried to use it
with registry authentication. Any objections to switching that to
commons-codec?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/spotify/docker-client/pull/340/files/f540f50f336c198eca7ea006a9f59e5a5f0f0e30#r61761412

yeah that was the first thing I thought of but thought it might be a bit presumptuous to update the jdk version in this PR :)

But resteasy does not throw hk2-exceptions, but jax-rs exceptions, so that is different right?

The only place the MultiException is used is in an multi-catch with ExecutionException which is what gets thrown by either implementation from what I can tell in the .get() call because it's async. In my tests it seems to propagate exceptions just fine. They get dealt with by the line
} else if (cause instanceof WebApplicationException) {...

Ah, missed that statement. Exception handlig should be fine then! 👍

jwgmeligmeyling · 2016-04-30T12:21:25Z

Great work! The PR looks quite good. There are a few remarks from my side (as a contributor, not maintainer).

It seems the code is still dependent on org.glassfish.hk2.api.MultiException and org.glassfish.jersey.internal.util.Base64. Besides that this implies that (at least some) glassfish dependencies are still required on the classpath, I think that exceptions may not be handled properly when another HttpClient implementation is used.

I would love to see a few additions:

Add resteasy-client (and its Jackson companion) as an optional and provided dependency:

<dependency>
    <groupId>org.jboss.resteasy</groupId>
    <artifactId>resteasy-client</artifactId>
    <version>3.0.16.Final</version>
    <scope>provided</scope>
    <optional>true</optional>
</dependency>

(In fact, this should also be done with the glassfish dependencies in my opinion, but that would be a breaking change for existing users, so lets stick to adding that as <exclusion> for now)

Alter the intergration tests in such as way (either through test class inheritance or for example JUnit theories or parameterized) that the same tests are ran for both the glassfish and resteasy-client. This would address my concern that some exception handling (or for example any timeout vs. non-timeout issues) are not working properly.
Some example usage of how to use another HttpClient can be used and specifically how the resteasy-client should be configured in practice would be nice!
The PR should be up to date with master, so it can be merged :)

jwgmeligmeyling · 2016-04-30T12:25:49Z

ping @grexe

jskjons · 2016-05-03T16:20:47Z

I can add the optional dependencies but I'm not sure if it really adds much value since the user still has to add those dependencies themselves anyway. Unless you're going to fully support multiple JAXRS implementations I'm not sure how much value there is in kinda-sorta supporting it. I mainly wanted to clear the roadblock preventing usage of anything else.

If you want to full on support multiple implementations I think ideally there should be a few more breaking changes like extracting the dependencies and factories into separate modules.

I'm happy to add an example usage of using RESTEasy with this, should that go in user_manual.md?

jwgmeligmeyling · 2016-05-03T16:30:07Z

The dependencies should be included anyway in order to be able to test the Resteasy client. My opinion is still that this should be tested properly, including timeout and exception behaviour. So I would like to see the integration tests to be ran on both client implementations.

The optional inclusion would mostly be a cosmetic feature indicating it can be used with other clients.

jskjons · 2016-05-03T16:35:10Z

fair enough, I'll make that change today.

jwgmeligmeyling · 2016-05-03T16:37:38Z

Allright. I can have a look on Thursday to maybe add some tests if you'd like :)

jskjons · 2016-05-04T00:07:05Z

I added the ResteasyClientFactory implementation as well as deps + docs. It turned out there was a few of the features that we were using (streaming stuff in particular) that was a bit of a pain to get working in RESTEasy but the integration tests are passing now. The Events stuff currently directly relies on jersey so it would be more difficult to get working so I just left it for now and documented that fact.

jwgmeligmeyling · 2016-05-04T06:19:44Z

Whoa, great work! Will defenitely start using our fork 😄 Any idea on the last failing test case?

Tests in error:
testEventStreamWithUntilTimeresteasy: The supplied component "org.jboss.resteasy.client.jaxrs.ResteasyClient" is not assignable from JerseyClient or JerseyWebTarget.

jwgmeligmeyling · 2016-05-04T06:21:51Z

  * [Exec Resize](#exec-resize)
  * [Exec Inspect](#exec-inspect)
  * [Mounting volumes in a container](#mounting-volumes-in-a-container)
+  * [Using RESTEasy instead of Jersey]#(resteasy)


Seems that the #( should be flipped to (# (preview)

jskjons · 2016-05-04T15:32:12Z

that last test was failing because the events stuff doesn't work with resteasy so I needed to add

assumeThat(sut.getClient(), not(instanceOf(ResteasyClient.class)));

to all of those tests and missed one. Should be good now.

jwgmeligmeyling · 2016-05-04T21:28:26Z

Great! Waiting on response from the maintainers then.

johnflavin · 2016-05-05T13:21:36Z

That test failure on 1.10.3 is caused by the same problem as #412. That issue (or at least the test that failed because of the issue) was fixed in #415. Rebase this PR on master and that issue should go away.

…lementations to be used

This required: - adding a few annotations - adding resteasy dependencies - adding hackery to make streaming work with RESTEasy - switching to Parameterized JUnit tests - documentation for how to use it

bitsofinfo · 2017-07-07T19:37:52Z

+1

mydeadlyvenoms · 2017-10-27T21:24:17Z

Any updates on this?

jskjons · 2017-10-27T21:27:22Z

I haven't had time to keep the branch up to date with master and I haven't heard anything from the maintainers so it appears this isn't a priority or not a direction the maintainers want to go it. We're still internally using this branch so it still works.

mydeadlyvenoms · 2017-10-27T21:29:56Z

I am having trouble to use the library under Glassfish / Payara due to Jersey conflicts - this might be the solution.

Rebasing the existing fork from current master branch proved too tedious for the purpose of this pull request. I need to test that the addition of the Ulimit builder will work with tenzing scripts and shipyard client. If this proves to be successful perhaps I can revisit merging all components of the master branch into this fork, but as this work is blocking another teammate, I want to free them up ASAP.

Adding Ulimit Builder object in HostConfig Builder.

rmannibucau · 2018-07-06T09:57:05Z

+1, i'd like to use CXF and JSONB instead of jackson, using portable API is really important

davidxia force-pushed the master branch from 7e6cb26 to 7a181cc Compare March 15, 2016 05:16

jskjons mentioned this pull request Apr 29, 2016

Migrate to clean JAX-RS 2.0 API and remove proprietary dependencies #413

Closed

jwgmeligmeyling reviewed Apr 30, 2016
View reviewed changes

jskjons force-pushed the master branch 2 times, most recently from eddee9f to 4d878f6 Compare May 2, 2016 21:47

jwgmeligmeyling reviewed May 4, 2016
View reviewed changes

jwgmeligmeyling mentioned this pull request May 4, 2016

Changed formatting for user manual rei/docker-client#1

Merged

jskjons and others added 4 commits May 6, 2016 08:42

extracting Client creation to interface to allow alternate client imp…

593aefe

…lementations to be used

complete support for RESTEasy + tests

610d83f

This required: - adding a few annotations - adding resteasy dependencies - adding hackery to make streaming work with RESTEasy - switching to Parameterized JUnit tests - documentation for how to use it

Changed formatting for user manual

5f5a4ed

ignoring all event stream tests for resteasy

cb39580

jskjons force-pushed the master branch from 128c40f to cb39580 Compare May 6, 2016 15:42

davidxia force-pushed the master branch from e4f80c8 to 6990f23 Compare May 25, 2016 04:17

This was referenced Mar 20, 2017

Getting NoSuchMethodError when using docker-client-2.7.7-shaded.jar #241

Closed

integrating PR #340 into latest docker-client #691

Open

johnflavin mentioned this pull request Jul 7, 2017

No message body reader has been found for class java.util.List, ContentType: application/json #820

Closed

leachad and others added 3 commits June 14, 2018 12:28

Updated the equals and hashcode methods

628be7b

Merge pull request #2 from rei/pr/update-host-config-with-ulimit-builder

c898d4f

Adding Ulimit Builder object in HostConfig Builder.

Conversation

jskjons commented Feb 4, 2016

Uh oh!

codecov-io commented Feb 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 62.78% (diff: 75.78%)

Uh oh!

jwgmeligmeyling Apr 30, 2016

Choose a reason for hiding this comment

Uh oh!

jskjons May 2, 2016

Choose a reason for hiding this comment

Uh oh!

grexe May 3, 2016

Choose a reason for hiding this comment

Uh oh!

jskjons May 3, 2016

Choose a reason for hiding this comment

Uh oh!

jwgmeligmeyling May 3, 2016

Choose a reason for hiding this comment

Uh oh!

jskjons May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwgmeligmeyling May 3, 2016

Choose a reason for hiding this comment

Uh oh!

jwgmeligmeyling commented Apr 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwgmeligmeyling commented Apr 30, 2016

Uh oh!

jskjons commented May 3, 2016

Uh oh!

jwgmeligmeyling commented May 3, 2016

Uh oh!

jskjons commented May 3, 2016

Uh oh!

jwgmeligmeyling commented May 3, 2016

Uh oh!

jskjons commented May 4, 2016

Uh oh!

jwgmeligmeyling commented May 4, 2016

Uh oh!

jwgmeligmeyling May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jskjons commented May 4, 2016

Uh oh!

jwgmeligmeyling commented May 4, 2016

Uh oh!

johnflavin commented May 5, 2016

Uh oh!

bitsofinfo commented Jul 7, 2017

Uh oh!

mydeadlyvenoms commented Oct 27, 2017

Uh oh!

jskjons commented Oct 27, 2017

Uh oh!

mydeadlyvenoms commented Oct 27, 2017

Uh oh!

rmannibucau commented Jul 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov-io commented Feb 5, 2016 •

edited

Loading

jskjons May 3, 2016 •

edited

Loading

jwgmeligmeyling commented Apr 30, 2016 •

edited

Loading

jwgmeligmeyling May 4, 2016 •

edited

Loading