Skip to content
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

[LIVY-416] Upgrade the version of Jackson from 2.4.4 to 2.9.2 #64

Closed
wants to merge 1 commit into from

Conversation

kjmrknsn
Copy link
Contributor

@kjmrknsn kjmrknsn commented Nov 17, 2017

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/LIVY-416

com.fasterxml.jackson.core.JsonGenerationException is sometimes thrown. The full stack trace is show on JsonGenerationException.txt.

This is because of the Jackson's bug (FasterXML/jackson-core#307) which was fixed at Jackson 2.7.7.

To fix this issue, the version of Jackson should be updated from 2.4.4 to the latest one (2.9.2).

In addition, com.google.guava:guava:15.0 was added to the dependencies because it was removed from the dependencies of com.fasterxml.jackson.module:jackson-module-scala_{2.10,2.11}:2.9.2.

How was this patch tested?

By executing mvn clean package.

@jerryshao
Copy link
Contributor

I guess your changes affect the classpath of integration test, which makes all the integration tests fail.

@kjmrknsn kjmrknsn force-pushed the LIVY-416 branch 4 times, most recently from 077703e to 1ec5bd3 Compare November 22, 2017 09:54
@codecov-io
Copy link

codecov-io commented Nov 22, 2017

Codecov Report

Merging #64 into master will increase coverage by 0.67%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #64      +/-   ##
============================================
+ Coverage     70.53%   71.21%   +0.67%     
- Complexity      800      803       +3     
============================================
  Files            97       97              
  Lines          5461     5461              
  Branches        801      801              
============================================
+ Hits           3852     3889      +37     
+ Misses         1081     1047      -34     
+ Partials        528      525       -3
Impacted Files Coverage Δ Complexity Δ
...ain/java/org/apache/livy/rsc/driver/RSCDriver.java 80% <0%> (+0.85%) 43% <0%> (+1%) ⬆️
...main/java/org/apache/livy/rsc/ContextLauncher.java 83.82% <0%> (+2.45%) 17% <0%> (ø) ⬇️
...cala/org/apache/livy/scalaapi/ScalaJobHandle.scala 55.88% <0%> (+2.94%) 0% <0%> (ø) ⬇️
...java/org/apache/livy/rsc/rpc/KryoMessageCodec.java 98.21% <0%> (+3.57%) 19% <0%> (+1%) ⬆️
...rg/apache/livy/repl/AbstractSparkInterpreter.scala 58.62% <0%> (+4.13%) 1% <0%> (ø) ⬇️
.../scala/org/apache/livy/repl/SparkInterpreter.scala 89.32% <0%> (+20.38%) 8% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64d71aa...60bd3af. Read the comment docs.

@kjmrknsn
Copy link
Contributor Author

Travis CI status turned to green: https://travis-ci.org/apache/incubator-livy/builds/305717408

@jerryshao
Copy link
Contributor

jerryshao commented Nov 22, 2017

Can you please explain why adding guava dependency will fix the previous issue? Guava is a transient dependency introduced by Jackson-module, not sure why do we need to add it explicitly?

@kjmrknsn
Copy link
Contributor Author

@jerryshao

Can you please explain why adding guava dependency will fix the previous issue? Guava is a transient dependency introduced by Jackson-module, not sure why do we need to add it explicitly?
Merge state

It's necessary to add guava to the dependencies explicitly, which is included in the dependencies of jackson-module-scala 2.4.4 because the livy-server project depends on guava in the following files and it is removed from the dependencies of jackson-module-scala 2.9.2.

  1. InteractiveSession.scala
  2. LineBufferedStream.scala

@kjmrknsn kjmrknsn force-pushed the LIVY-416 branch 3 times, most recently from f106064 to e5f2fa9 Compare November 23, 2017 02:23
@jerryshao
Copy link
Contributor

LGTM, thanks for explanation.

@jerryshao
Copy link
Contributor

Travis is not so stable, you need to trigger several times to make it pass.

@kjmrknsn
Copy link
Contributor Author

@jerryshao

Travis is not so stable, you need to trigger several times to make it pass.

Thanks. The status turned to green: https://travis-ci.org/apache/incubator-livy/builds/306139499

@jerryshao
Copy link
Contributor

Thanks, let me merge to master.

@jerryshao
Copy link
Contributor

Because of sync issue between github and apache git, this jira merged in apache (https://git-wip-us.apache.org/repos/asf?p=incubator-livy.git;a=shortlog;h=refs/heads/master) didn't sync to github. It will be fixed automatically by pushing new commits.

hubot pushed a commit that referenced this pull request Nov 23, 2017
## What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/LIVY-416

`com.fasterxml.jackson.core.JsonGenerationException` is sometimes thrown. The full stack trace is show on [JsonGenerationException.txt](https://github.com/apache/incubator-livy/files/1482566/JsonGenerationException.txt).

This is because of the Jackson's bug (FasterXML/jackson-core#307) which was fixed at Jackson 2.7.7.

To fix this issue, the version of Jackson should be updated from 2.4.4 to the latest one (2.9.2).

In addition, `com.google.guava:guava:15.0` was added to the dependencies because it was removed from the dependencies of `com.fasterxml.jackson.module:jackson-module-scala_{2.10,2.11}:2.9.2`.

* [com.fasterxml.jackson.module:jackson-module-scala_2.10:2.4.4](https://mvnrepository.com/artifact/com.fasterxml.jackson.module/jackson-module-scala_2.10/2.4.4)
* [com.fasterxml.jackson.module:jackson-module-scala_2.11:2.4.4](https://mvnrepository.com/artifact/com.fasterxml.jackson.module/jackson-module-scala_2.11/2.4.4)
* [com.fasterxml.jackson.module:jackson-module-scala_2.10:2.9.2](https://mvnrepository.com/artifact/com.fasterxml.jackson.module/jackson-module-scala_2.10/2.9.2)
* [com.fasterxml.jackson.module:jackson-module-scala_2.11:2.9.2](https://mvnrepository.com/artifact/com.fasterxml.jackson.module/jackson-module-scala_2.11/2.9.2)

## How was this patch tested?
By executing `mvn clean package`.

Author: Keiji Yoshida <kjmrknsn@gmail.com>

Closes #64 from kjmrknsn/LIVY-416.
@kjmrknsn
Copy link
Contributor Author

Thanks for merging. May I close this PR?

@jerryshao
Copy link
Contributor

Yes, please.

@kjmrknsn kjmrknsn closed this Nov 23, 2017
@kjmrknsn kjmrknsn deleted the LIVY-416 branch November 29, 2017 04:26
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.

None yet

3 participants