-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-12154] Upgrade to Jersey 2 #11223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Changes to the Spark Web UI code were required to compile. The changes were relatively standard Jersey migration things.
|
Work in progress - want to do some more testing. In particular, I haven't tried the security filter. I'm not very familiar with web server development so definitely would appreciate feedback. |
|
Test build #51380 has finished for PR 11223 at commit
|
|
Test build #51389 has finished for PR 11223 at commit
|
|
Test build #51391 has finished for PR 11223 at commit
|
docker-integration-tests/pom.xml
Outdated
| <groupId>org.glassfish.jersey.core</groupId> | ||
| <artifactId>jersey-server</artifactId> | ||
| <version>1.19</version> | ||
| <version>2.22.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version should be declared once in <properties>, but generally, dependencies and versions are only managed in the parent POM
|
I'm not sure how to test the security filter things. Does anyone have suggestions of what features I should test around the UI filter that would exercise this code path? Something related to this feature I presume? https://spark.apache.org/docs/latest/security.html#web-ui |
|
Test build #51454 has finished for PR 11223 at commit
|
|
Test build #51506 has finished for PR 11223 at commit
|
|
Jenkins, retest this please |
|
Test build #51515 has finished for PR 11223 at commit
|
|
@JoshRosen the dependency change is pretty scary (adding a lot of libraries). Is this change necessary? |
| jcl-over-slf4j-1.7.10.jar | ||
| jdo-api-3.0.1.jar | ||
| jersey-client-1.9.jar | ||
| jersey-client-2.22.1.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have some Jersey and Jackson version problems here. Some Jackson deps are 1.9 some are 2.5. It's annoying but I think we would have to meticulously harmonize it. Jersey likewise... I'm assuming these are not the Sun jersey jars that appear as version 1.9? The javax.inject duplication would ideally be resolved too.
The additions themselves don't look too bad.
Do the Sun jersey classes belong to a different namespace? then I think we may need to not exclude them (but not use them) since they may be present for third party libs.
Overall, I like updating things like this for a major release, though I don't yet fully have my head around the implications at runtime, where another Jackson version or Jersey version might turn up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble with including both com.sun.jersey and org.glassfish.jersey is that you end up with multiple javax.ws.rs versions on the classpath - com.sun.jersey packages javax.ws.rs classes internally while org.glassfish.jersey requires providing the javax.ws.rs classes externally. This led to the discussion around SPARK-11081 - see #9615.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I missed some exclusions for com.sun.jersey, so completing the exclusions appears to have made a lot of the mismatching versions go away. How does it look now?
|
Test build #51695 has finished for PR 11223 at commit
|
|
@mccheah how's this going? What more do you need to do to get this in? |
|
Just need reviewing, as well as advice on how to test the UI Security Filter, as that's a feature I'm unfamiliar with. Also do we have unit tests that would be wrapping this kind of code or should I write something new? |
|
This needs a few things -- a rebase of course. Next I'm concerned with dependency harmonization. For example I see differing versions of Jackson dependencies being pulled in. I think I expect to see both 1.x and 2.x dependencies, but not different versions of 1.x or 2.x. Likewise with Jersey, it seems like a few 1.x dependencies are still coming in. Figuring this out is the painful part. We also need to update LICENSE files to reflect new dependencies. More tests can't hurt; I don't know how to test the security aspect. Tests do in general basically exercise the web UI already. |
|
I don't think I see any specific Jackson dependencies with different versions, but different Jackson modules have different versions being pulled in. How would I go about finding out if these different versions with play nicely with one another? |
|
For example: I know we're going to have 1.x and 2.x pulled in, but the 2.x versions don't totally align. I wouldn't mind managing them all to the same version for tidiness, even if it's ultimately a game of whack-a-mole. I don't have any specific reason to think 2.5.3 and 2.5.4 don't work, but if the Spark version goes to 2.6.x while others stay on 2.5.3 it could be a problem. |
|
@mccheah are you still able to pursue this? |
|
Apologize for not following up here. Since writing the initial change I think there may have been a lot of shifting from underneath (merge conflicts) so I'm going to close this and rewrite it. I'm going to fix all the Jackson versions to the same 2.x dependency. |
|
Moved to #12715 |
Changes to the Spark Web UI code were required to compile. The changes
were relatively standard Jersey migration things.