Skip to content

Conversation

@dflemstr
Copy link
Contributor

@dflemstr dflemstr commented Aug 7, 2014

I want to use this library in an environment where there are other libraries that depend on Jersey 2. Since Jersey 2 has better performance and a nicer API compared to Jersey 1, I thought I would just port this library to Jersey 2.

All the tests pass so I assume that this port is complete.

@marcuslinke
Copy link
Contributor

@dflemstr Great work! Big thanks for your effort! However i don't think i should merge it as is because some docker-java client code may be tied to jersey 1.x when using third party REST APIs with jersey 1.x also. This is because there seems to be problems having jersey 1.x and jersey 2.x in the same classpath (http://jersey.576304.n2.nabble.com/Repackaging-Jersey-1-x-jars-without-jax-rs-td7580810.html).

So i don't want to simply override the old jersey 1.x implementation. Rather it would be better to factor out an REST framework independent docker-java-api and provide two different implementations of it. There is a little discussion about it here: https://groups.google.com/forum/?hl=de#!topic/docker-java-dev/p_m9VIRrCeU

So as soon as i factored out this docker-java-api i will merge your branch.

Regards

@dflemstr
Copy link
Contributor Author

dflemstr commented Aug 7, 2014

It might be worth noting that that problem is already solved in some ways. Instead of using Jersey directly, the commands in this PR use only the normal JAX-RS client API. That means that any implementation of the JAX-RS API (e.g. RestEasy, CXF, Restlet) can be dropped in instead of Jersey, and the only thing that needs to be changed is how the Client is constructed in DockerClient.

So if you are looking for a "REST-framework independent API" then that's exactly what JAX-RS is already.

But you are correct in that it wouldn't allow you to use Jersey 1, since that library uses a proprietary Sun API. However, if I could choose, I myself would prefer to drop support for Jersey 1 and gain support for any JAX-RS 2 implementation, instead of having to create something similar to JAX-RS 2 and retain support for Jersey 1.

@marcuslinke
Copy link
Contributor

Yes, that would be the much simpler solution but currently i don't want drop Jersey 1.x support if possible. But maybe that needs to much effort. Regardless of it i think it will be a better design to have a docker-java API that not relies on any JAX-RS (1/2) class.

However, as i said already, big thanks for this PR!

@mfulgo
Copy link
Contributor

mfulgo commented Aug 7, 2014

@dflemstr Nicely done; I like it. (Just because all of the integration tests pass doesn't necessarily mean it's 100% correct: There are definitely code coverage gaps. Nevertheless...)

This is great because it breaks from the com.sun.* classes in favor of the more standard javax.* classes.

@marcuslinke I'd say this is a good step in the right direction, even if we later remove the exposure of the javax.* classes in the method/command return types. If someone really wants Jersey 1.x support, they could always use 0.9.1.

@dflemstr
Copy link
Contributor Author

dflemstr commented Aug 8, 2014

@marcuslinke I look forward to a more general solution for this. Meanwhile, we want to use this library at Spotify so I will maintain an internal fork. I suspect that the API won't change with your proposed solution?

@marcuslinke
Copy link
Contributor

@dflemstr The current 0.10.0-SNAPSHOT is intended to make some major refactorings, therefore i switched from 0.9.1 to 0.10.0. It's still in progress, so unfortunately the API will change for sure, but it will include package renaming and of course the client initialization step only. So reorganizing imports and change your client setup code should be sufficient.

@marcuslinke
Copy link
Contributor

Maybe i'm able to commit the refactored API until tomorrow, so that you could pull and integrate in your branch already.

@marcuslinke
Copy link
Contributor

@dflemstr , @mfulgo: After thinking about it again keeping jersey 1.x support will be to much effort, so i decided to drop support for it. The most problems will be with the test cases. I really don't want to introduce abstract test cases. Nevertheless i will merge the API changes i've already done with your branch @dflemstr.

@marcuslinke
Copy link
Contributor

@dflemstr Just merged your changes manually.

@marcuslinke
Copy link
Contributor

@dflemstr May you take a look at #52 please?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants