[SPARK-12154] Upgrade to Jersey 2 by mccheah · Pull Request #12715 · apache/spark

mccheah · 2016-04-26T21:51:46Z

What changes were proposed in this pull request?

Replace com.sun.jersey with org.glassfish.jersey. Changes to the Spark Web UI code were required to compile. The changes were relatively standard Jersey migration things.

How was this patch tested?

I did a manual test for the standalone web APIs. Although I didn't test the functionality of the security filter itself, the code that changed non-trivially is how we actually register the filter. I attached a debugger to the Spark master and verified that the SecurityFilter code is indeed invoked upon hitting /api/v1/applications.

Changes to the Spark Web UI code were required to compile. The changes were relatively standard Jersey migration things.

SparkQA · 2016-04-26T21:53:44Z

Test build #57035 has finished for PR 12715 at commit b86ed6f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

mccheah · 2016-04-26T21:53:45Z

+javassist-3.18.1-GA.jar
+javax.annotation-api-1.2.jar
 javax.inject-1.jar
+javax.inject-2.4.0-b34.jar


Is it problematic to have both javax.inject-1 and javax.inject-2.4.0?

I wouldn't say problematic, but both provide the exact same set of classes, so it's at least redundant. Being a javax API, I at least assume the newer one is backwards compatible.

SparkQA · 2016-04-26T22:05:59Z

Test build #57039 has finished for PR 12715 at commit b4d72d8.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

mccheah · 2016-04-26T23:47:00Z

 <dependency>
- <groupId>com.sun.jersey</groupId>
- <artifactId>jersey-core</artifactId>
+ <groupId>org.glassfish.jersey.core</groupId>


Actually this doesn't work because the Mini YARN cluster needs Jersey 1 to start up. Since this is only in test scope we should be fine to keep the Jersey 1 dependency here - I'll add a comment to clarify that.

SparkQA · 2016-04-27T00:37:06Z

Test build #57041 has finished for PR 12715 at commit e2dbf03.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-27T01:28:45Z

Test build #57069 has finished for PR 12715 at commit 5793600.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-27T03:13:59Z

Test build #57067 has finished for PR 12715 at commit 313401b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-04-27T03:16:52Z

retest this please

SparkQA · 2016-04-27T03:28:04Z

Test build #57070 has finished for PR 12715 at commit c9b5c3b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-27T05:17:27Z

Test build #57082 has finished for PR 12715 at commit c9b5c3b.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2016-04-27T08:22:56Z

 antlr-runtime-3.4.jar
 antlr4-runtime-4.5.2-1.jar
 aopalliance-1.0.jar
+aopalliance-repackaged-2.4.0-b34.jar


We'll probably have to look at the impact on the LICENSE file for all these dependency changes. I can help do that bit if once we're pretty sure this will work.

mccheah · 2016-04-27T18:25:05Z

@JoshRosen I did not change the Jersey dependency for the docker-integration-tests project. Should we change that here or leave it alone?

mccheah · 2016-04-27T18:26:28Z

Jenkins, retest this please

SparkQA · 2016-04-27T20:19:18Z

Test build #57155 has finished for PR 12715 at commit c9b5c3b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-27T21:15:25Z

Test build #57174 has finished for PR 12715 at commit bed5618.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-27T21:20:14Z

Test build #57177 has finished for PR 12715 at commit bfb178a.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-28T00:06:11Z

Test build #57182 has finished for PR 12715 at commit a199cb6.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

mccheah · 2016-04-28T00:29:19Z

Hm... Are SparkLauncher tests just flaky or could this change have directly affected that?

mccheah · 2016-04-28T00:29:25Z

Jenkins, retest this please

vanzin · 2016-04-28T00:31:12Z

That particular one shouldn't be affected by these changes. I'll take a closer look at it separately.

SparkQA · 2016-04-28T00:42:37Z

Test build #57184 has finished for PR 12715 at commit 5a95559.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-28T02:49:21Z

Test build #57196 has finished for PR 12715 at commit 5a95559.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-04-28T18:26:37Z

LGTM, I'll leave this for @srowen since he had a comment about licenses.

srowen · 2016-04-29T14:17:46Z

Yeah you can update the following section of NOTICE to read as follows and that should be it:

========================================================================
Common Development and Distribution License 1.0
========================================================================
The following components are provided under the Common Development and Distribution License 1.0. See project link for details.
(CDDL 1.0) Glassfish Jasper (org.mortbay.jetty:jsp-2.1:6.1.14 - http://jetty.mortbay.org/project/modules/jsp-2.1)
(CDDL 1.0) JAX-RS (https://jax-rs-spec.java.net/)
(CDDL 1.0) Servlet Specification 2.5 API (org.mortbay.jetty:servlet-api-2.5:6.1.14 - http://jetty.mortbay.org/project/modules/servlet-api-2.5)
(CDDL 1.0) (GPL2 w/ CPE) javax.annotation API (https://glassfish.java.net/nonav/public/CDDL+GPL.html)
(COMMON DEVELOPMENT AND DISTRIBUTION LICENSE (CDDL) Version 1.0) (GNU General Public Library) Streaming API for XML (javax.xml.stream:stax-api:1.0-2 - no url defined)
(Common Development and Distribution License (CDDL) v1.0) JavaBeans Activation Framework (JAF) (javax.activation:activation:1.1 - http://java.sun.com/products/javabeans/jaf/index.jsp)
========================================================================
Common Development and Distribution License 1.1
========================================================================
The following components are provided under the Common Development and Distribution License 1.1. See project link for details.
(CDDL 1.1) (GPL2 w/ CPE) org.glassfish.hk2 (https://hk2.java.net)
(CDDL 1.1) (GPL2 w/ CPE) JAXB API bundle for GlassFish V3 (javax.xml.bind:jaxb-api:2.2.2 - https://jaxb.dev.java.net/)
(CDDL 1.1) (GPL2 w/ CPE) JAXB RI (com.sun.xml.bind:jaxb-impl:2.2.3-1 - http://jaxb.java.net/)
(CDDL 1.1) (GPL2 w/ CPE) Jersey 2 (https://jersey.java.net)

srowen · 2016-05-01T08:51:49Z

 <groupId>com.sun.jersey</groupId>
 <artifactId>jersey-core</artifactId>
 <scope>test</scope>
+ <version>1.9</version>


One last finishing touch .. you could define (in just this pom) a property for Jersey 1.x versions to make sure they're modified together.

mccheah · 2016-05-03T09:35:24Z

@srowen Should the NOTICE still contain Jersey 1 since I think we still pull it in explicitly for YARN unit tests?

srowen · 2016-05-03T11:55:14Z

This is looking good. Regarding the license, it depends on whether the artifacts we distribute contain Jersey 1.x or not. I have the impression it isn't at the moment. If you have the build handy and can look at the assembly and grep its contents, that would help verify whether 1.x classes are still there or not. If not, this is fine. Otherwise just restore the Jersey 1.x license statements.

SparkQA · 2016-05-03T11:56:37Z

Test build #57622 has finished for PR 12715 at commit 70d4844.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

mccheah · 2016-05-03T17:48:59Z

I didn't see any old-Jersey jars in the lib directory.

mccheah · 2016-05-04T08:12:31Z

@srowen given that Jersey 1 isn't being distributed, this PR should be good to go?

srowen · 2016-05-04T09:46:47Z

LGTM; let me leave this open a short while longer for comments.

fabriziocucci · 2016-05-04T12:53:10Z

Question: will this be ported to the 1.6 branch?

srowen · 2016-05-04T12:55:13Z

No, it's too significant a change for a maintenance release.

fabriziocucci · 2016-05-04T18:24:18Z

Thanks Sean. So, just to recap regarding this, whoever is stuck trying to use spark in a web application based on Jersey 2 has currently the following options:

wait for spark 2.0;
rollback the web application to use Jersey 1.9 (to be consistent with spark);
rollback the spark version to be 1.3.1 (the only one that can be easily integrated by simply excluding the jersey-server dependecy);
replace Jersey 2 in the web application with something else (e.g. spring mvc).

Is my understanding correct?

srowen · 2016-05-04T18:28:38Z

I think the best option is to shade Jersey if you need a different version.

srowen · 2016-05-05T09:51:35Z

Merged to master/2.0

## What changes were proposed in this pull request? Replace com.sun.jersey with org.glassfish.jersey. Changes to the Spark Web UI code were required to compile. The changes were relatively standard Jersey migration things. ## How was this patch tested? I did a manual test for the standalone web APIs. Although I didn't test the functionality of the security filter itself, the code that changed non-trivially is how we actually register the filter. I attached a debugger to the Spark master and verified that the SecurityFilter code is indeed invoked upon hitting /api/v1/applications. Author: mcheah <mcheah@palantir.com> Closes#12715 from mccheah/feature/upgrade-jersey. (cherry picked from commit b7fdc23) Signed-off-by: Sean Owen <sowen@cloudera.com>

[SPARK-12154] Upgrade to Jersey 2
b86ed6f
Changes to the Spark Web UI code were required to compile. The changes were relatively standard Jersey migration things.

mccheah reviewed Apr 26, 2016
View reviewed changes

Move more jersey things from kinesis-assembly
b4d72d8

Add jersey servlet container to manifests
e2dbf03

mccheah mentioned this pull request Apr 26, 2016
[SPARK-12154] Upgrade to Jersey 2 #11223
Closed

mccheah reviewed Apr 26, 2016
View reviewed changes

mccheah added 3 commits April 26, 2016 18:21

Use com.sun.jersey dependencies in YARN tests
313401b

Clean up
5793600

Fix tests
c9b5c3b

srowen reviewed Apr 27, 2016
View reviewed changes

mccheah added 2 commits April 27, 2016 14:08

Remove Jersey dependency from hive-thriftserver
bed5618

Mark Kafka jersey dependencies as provided
bfb178a

mccheah added 2 commits April 27, 2016 15:04

Remove Jersey dependency from kafka-assembly
a199cb6

Remove kafka jersey dependency properly
5a95559

srowen reviewed May 1, 2016
View reviewed changes

mccheah added 4 commits May 3, 2016 11:39

Update NOTICE for Jersey 2 dependency
48861d8

Factor out old Jersey version for the YARN project
19d6ea2

Merge remote-tracking branch 'origin/master' into upgrade-jersey
eea09b1

Fixing merge conflict by updating deps
70d4844

asfgit closed this in b7fdc23May 5, 2016

robert3005 deleted the feature/upgrade-jersey branch September 24, 2016 04:09

Uh oh!

Conversation

mccheah commented Apr 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Apr 26, 2016

Uh oh!

mccheahApr 26, 2016

Choose a reason for hiding this comment

Uh oh!

vanzinApr 28, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 26, 2016

Uh oh!

mccheahApr 26, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

vanzin commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

srowenApr 27, 2016

Choose a reason for hiding this comment

Uh oh!

mccheah commented Apr 27, 2016

Uh oh!

mccheah commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 27, 2016

Uh oh!

SparkQA commented Apr 28, 2016

Uh oh!

mccheah commented Apr 28, 2016

Uh oh!

mccheah commented Apr 28, 2016

Uh oh!

vanzin commented Apr 28, 2016

Uh oh!

SparkQA commented Apr 28, 2016

Uh oh!

SparkQA commented Apr 28, 2016

Uh oh!

vanzin commented Apr 28, 2016

Uh oh!

srowen commented Apr 29, 2016

Uh oh!

srowenMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

mccheah commented May 3, 2016

Uh oh!

srowen commented May 3, 2016

Uh oh!

SparkQA commented May 3, 2016

Uh oh!

mccheah commented May 3, 2016

Uh oh!

mccheah commented May 4, 2016

Uh oh!

srowen commented May 4, 2016

Uh oh!

fabriziocucci commented May 4, 2016

Uh oh!

srowen commented May 4, 2016

Uh oh!

fabriziocucci commented May 4, 2016