[SPARK-11081] Shade Jersey and javax.rs.ws #9615
Conversation
Jersey and javax.rs.ws are commonly depended on and cause dependency conflicts in many applications that depend on the Spark maven artifacts. This patch attempts to shade them.
|
Test build #45586 has finished for PR 9615 at commit
|
| @@ -2165,6 +2166,9 @@ | |||
| <include>org.eclipse.jetty:jetty-security</include> | |||
| <include>org.eclipse.jetty:jetty-util</include> | |||
| <include>org.eclipse.jetty:jetty-server</include> | |||
| <include>com.sun.jersey:jersey-core</include> | |||
| <include>com.sun.jersey:jersey-json</include> | |||
| <include>com.sun.jersey:jersey-server</include> | |||
There was a problem hiding this comment.
Do you also need to add the javax.ws.rs artifacts to this <includes> list?
There was a problem hiding this comment.
javax.ws.rs is actually inside the Jersey artifacts.
The shading didn't work as I expected regardless if I added the javax.rs.ws stuff to here as well though.
There was a problem hiding this comment.
Hey @mccheah - if you look at jetty-server as an example, there are other build changes related to shading that you haven't done here:
- In the root pom, they should be marked as provided.
- In core/pom.xml they need to be added in includeArtifactIds.
- They may also need to be listed in other poms as well due to some compiler bugs.
I'd just go through and look everywhere in the source code you see jetty-server and do the same for these 3 artifacts. If it's still not working then, let me know and I can take a look.
> git grep jetty-server
core/pom.xml: <artifactId>jetty-server</artifactId>
core/pom.xml: guava,jetty-io,jetty-servlet,jetty-continuation,jetty-http,jetty-plus,jetty-util,jetty-server,jetty-security
pom.xml: <artifactId>jetty-server</artifactId>
pom.xml: <include>org.eclipse.jetty:jetty-server</include>
repl/pom.xml: <artifactId>jetty-server</artifactId>
streaming/pom.xml: <artifactId>jetty-server</artifactId>
yarn/pom.xml: <artifactId>jetty-server</artifactId>
|
Another thing to keep in mind, given the recent issues with The good news is that since new versions of these APIs are backwards compatible, you shouldn't need to shade the Given that, I wonder if it's even necessary to shade jersey at all; the version of jersey used in Spark is old and doesn't clash with the new package name used by newer releases of Jersey. |
|
@vanzin we've hit cases specifically where swapping in the newer version of javax.rs.ws causes the Spark UI to break. I'm not convinced that javax.rs.ws versions are compatible between what Spark uses and the newer versions. @pwendell great I'll try these suggestions. Apologies for my lack of knowledge, but what are the sorts of things I should be testing with regards to this change? That is, what Spark components are specifically using the javax.rs.ws and Jersey stuff? I know the Spark UI is one of them, is there anything else? I'm also inspecting the jar files after building. |
|
Regarding testing, the best way is to inspect the contents of the jar to make sure that the shaded version is inlined. If Spark code uses the shaded dependency directly, you can also use @mccheah given the comments by @vanzin, can you say more about the specific incompatibility you are facing? It would be good to make sure that if we shade something it's due to a known incompatibility between some set of versions. |
|
My concern is specifically with shading the annotations; we've seen that it causes issues with scalac-generated code. If you manage to fix the PR to shade jax-rs annotations, please do extensive tests with spark-shell to make sure it still works. I'm also interested in which incompatibilities you ran into. These APIs are generally pretty good at maintaining backwards compatibility; unless you're using some milestone build (I remember pre-release jars of the JAX-RS 2.0 API having some differences from the final version), it should all work. |
|
We're seeing that exception handling has changed between Jersey 01 and Jersey 02. In our application, we are including both Jersey 2 and javax.ws.rs-api-2.0.1.jar on the driver and executor classpath. I simplified it to a Spark shell session: We have other examples as well where similar errors are happening. I think in this case, |
|
What does everyone think about the case I've outlined above? |
|
You cannot use |
|
I used And this time I got... Edit: Using --jars and --conf spark.driver.userClassPathFirst=true led to the same NoSuchMethodError I listed above. |
|
The separator for |
|
The NoSuchMethodError occurs if I use --driver-class-path with the right delimiter. And regardless - should we be shading so that users shouldn't have to set the userClassPathFirst settings on drivers and/or executors? Edit: Actually I looked more closely at my exception trace. It's a different error, but it still doesn't work: |
But you stated before that shading wasn't yet fully done by this patch? Is that not the case anymore? I've stated what my worry is: JAX-RS is based on annotations and shading annotations tends to break scalac-generated code. If you have fixed the shading, you need to (i) build spark using maven and (ii) test that everything, especially the spark-shell, still works. Please refer to the PR I linked to before (#9585). The PR builder is not enough, since even if you trigger it with maven, it adds too many things to the test classpath, preventing the error from happening. |
|
Ah, I haven't actually fixed the shading yet - despite adding a bunch of logic to pom files the classes weren't being included in the assembly jar the way I expected. However I posted the reproduction cases to demonstrate the need for shading in the first place. Are we all on the same page with the reason why we need to do the shading here? If we're all on board I'll post what I've tried and we can debug further from there. |
|
Ok, so I figured out why you were seeing the exceptions with The old version of jersey hooks up to the JAX-RS API using The old jersey implementation of the This also means that plain shading will not work. You would have to modify all the services files too; not sure whether the plugins does that, but I don't think so. And that's assuming that shading JAX-RS annotations doesn't break other things. One thing I tried and seems to have worked in a quick test is to not package the old jersey service files in the assembly. I just added this to the sbt build: When I added the new jersey to the class path, the new classes were loaded (and worked), and the Spark UI was still able to serve things under |
|
Hm... based on what we've found so far, it sounds like shading Jersey is a pretty difficult task. In general any scenario where the application code uses a different version of Jersey than what Spark uses will be a painful experience. Based on this, does it perhaps make sense to try to remove our dependency on Jersey entirely? What are we using Jersey for? Would it be a significant undertaking to replace the parts that use Jersey with something else? (This was also suggested on the JIRA comments) |
Jersey is used to serve the JSON API. See http://spark.apache.org/docs/latest/monitoring.html, under "REST API". But, if you paid attention to my investigation, it seems that if you remove the service files from the old jersey, it should be possible to use "userClassPathFirst" to override it for the application, which is how you should override libraries used by Spark anyway. That doesn't require shading anything. |
|
But if you remove the META-INF/service files, but the user doesn't ship / use Jersey, then, the Jersey 1 stuff won't work, right? What are the implications for users that don't depend on Jersey? Edit: I see that "without the new classes, /api also works" - but what is Jersey falling back to then when the META-INF/services part is being excluded? Also, shouldn't we be aiming for a solution that doesn't require the userClassPathFirst setting? |
|
I did not remove the old jersey classes, just the If that really works, then it opens a door to allowing users to override jersey in their apps. My quick test worked, but more thorough testing would need to be done. |
|
I think I have a different issue cropping up that won't be solved by just not including the META-INF/services/javax.ws.rs files in the assembly. For context, my application is pulling in the spark-* artifacts as maven dependencies, and just spawning a Spark context in its JVM to connect to a Spark cluster. I ran the following experiment with my app:
(Side note: 1.4.1-palantir3 is just a few extra patches that are in master/1.5+ on top of 1.4.1. https://github.com/palantir/spark/commits/v1.4.1-palantir3) |
Yeah, that's a different use case, which isn't really covered by my suggestions. For that case, a different approach might be needed. Instead of shading, a small pom file could be created to create a spark-specific version of the old jersey artifacts without the offending files in META-INF. That way, when you pull jersey transitively via spark-core, you'd be getting this sanitized jar instead of the original one. Another approach could be to upgrade the version of jersey used by Spark. I am not particularly a fan of upgrading dependencies, but it's unlikely people depend on that dependency currently. |
|
The exception we're getting here seems like it's a namespace-difference between Jersey 1 and Jersey 2, and Spark is trying to use Jersey 1's namespace when the class has been moved. Upgrading to Jersey 2 is an option; if we shade Jersey 2 - which from my understanding doesn't get us tripped up by META-INF/services, because Jersey 2 doesn't use that - then can we get a solution that doesn't force Jersey users to specify userClassPathFirst? |
|
The exception you're getting is because you explicitly removed the jersey 1 dependency, right? My suggestion would keep the classes, get rid of the META-INF stuff. So you'd have both jerseys in your app, Spark using the old one, your app using the new one.
If possible, I prefer not shading anything. |
|
To clarify, does this solution require that I load the jersey 2 jar before the jersey 1 jar in order to make sure the javax.rs.ws classes are loaded from jersey 2 in my app? Or are we saying that if META-INF/services is purged from Jersey 1 then it doesn't matter which javax.ws.rs classes are loaded? |
|
If you purge the META-INF stuff, you'll just need to make sure your app has the newest javax.ws.rs classes available; that may or may not mean adding exclusions when adding a spark-core dependency, depending on the artifact names. The order the jersey 1 and 2 jars are included does not matter, since they do not conflict with each other. |
|
I don't think the javax.ws.rs classes are actually compatible with each other. I just tried another experiment. I took my Spark package hacked to remove the META-INF/services/javax.ws.rs* files. I then ran this: |
|
That's because jersey 2 has a bunch of other dependencies you need aside from the two jars you're adding. If you find all of them and add them to the classpath, then it will work. |
|
So I tried the following experiment to simulate what we want to be doing and testing the hypothesis that it doesn't matter what order the Jersey jars are loaded in. What I did was, take all of the jersey 1 jars in my application's classpath, and remove the META-INF/services/javax.ws.* files from them. I also renamed my jersey-1 jars, prefixing them with 1-jersey-*. I started my application and on the driver side I created a javax.ws.rs.NotFoundException, and hit this error: javax.ws.rs.NotFoundException is a class in the Jersey 2 jar, but it extends javax.ws.rs.ClientErrorException... which in turn extends javax.ws.rs.WebApplicationException. The WebApplicationException class exists in both Jersey jars but are different. In particular, WebApplicationException in the Jersey 2 jar contains the validate() method and that method is absent in Jersey 1. WebApplicationException was loaded from Jersey 1 and so it is missing the validate() method that ClientErrorException tries to call. The NoSuchMethodError didn't manifest if I didn't rename the Jersey jars, indicating that class loading order indeed does matter here. |
|
Seems like the old jersey-core jar has the javax classes within it, instead of depending on a separate artifact; so you'd need the new javax jar in front of it in the classpath. If going with the "modify Spark build" path, a similar approach to removing META-INF files could be used to create a separate jar for the javax api only, that could then be excluded from spark-core dependencies if you want to. |
|
So to recap this approach holistically, both from the perspective of my application and from what needs to change Spark side:
Am I missing anything? Thanks a lot by the way @vanzin , this discussion has been extremely helpful. |
|
Steps 1 and 4 are both needed for your app, since you're embedding Spark. Basically "org.spark-project.jersey" can't contain either javax.ws classes nor the META-INF files. |
|
My app would depend on Jersey 2 only which doesn't package the javax.ws.rs in itself and doesn't have the META-INF/services/javax.ws.rs* stuff. |
|
But it also depends on spark-core, no? You said so above. And spark-core would depend on the spark-specific jersey jar which in your list would have the META-INF classes... |
|
No - spark-specific Jersey jar would not have javax.ws.rs classes. That would be in org.spark-project.javax.ws.rs. Edit: spark-specific jersey jar would also exclude META-INF/service/javax.ws.rs* files and put those in org.spark-project.javax.ws.rs. |
|
But it (and spark-core) need to depend on that jar containing the javax stuff... |
|
No because my app would replace org.spark-project.javax.ws.rs with its own javax.ws.rs (which is transitively pulled in by Jersey 2 anyways), and those newer javax.ws.rs classes should be usable by org.spark-project.jersey as well. Although I think this particular part of the compatibility story is something we'll have to confirm in testing, or reason about a bit more precisely. |
|
Your app is one app. This needs to work for all apps. If spark-core doesn't depend on the artifact that has the javax classes, Spark won't work. |
|
Spark-core would depend on the org.spark-project.javax.ws.rs but I would exclude it in my app specifically, as well as anyone who would use Jersey 2. |
|
I see. I somehow missed that you were including the META-INF files in the javax jar. Anyway, I'm pretty sure the current code in this PR won't work; would you mind updating the bug with your latest summary above, and closing this PR? |
|
Hm alright. We could also update this PR inline but we can also just link to it for the discussion's sake in the follow-up PR. Closing! Thanks again! |
Jersey and javax.rs.ws are commonly depended on and cause dependency
conflicts in many applications that depend on the Spark maven artifacts.
This patch attempts to shade them.
However this patch set is a work in progress. Currently, the javax.rs.ws shading appears to be broken. When I run the mvn package task and generate the assembly jar, and unpack it, I'm missing the javax.rs.ws classes - they're not under org/spark-project/javax/rs/ws. I would appreciate some assistance in debugging how the shading is being done incorrectly here.