Skip to content

Upgrade jackson libraries to 2.12.1 #886

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

Closed
wants to merge 2 commits into from

Conversation

SerCeMan
Copy link

@SerCeMan SerCeMan commented Jan 6, 2021

Reference: #885

FasterXML has recently version released jackson version 2.12.0 libraries, the dependency can be upgraded.

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2021

CLA assistant check
All committers have signed the CLA.

@SerCeMan SerCeMan mentioned this pull request Jan 6, 2021
@codecov-io
Copy link

codecov-io commented Jan 8, 2021

Codecov Report

Merging #886 (e9c02b6) into develop (99ba86b) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #886      +/-   ##
===========================================
- Coverage    78.38%   78.32%   -0.06%     
===========================================
  Files          837      837              
  Lines        24802    24896      +94     
  Branches      1577     1582       +5     
===========================================
+ Hits         19441    19500      +59     
- Misses        5361     5396      +35     
Impacted Files Coverage Δ
...scala/com/twitter/finagle/mysql/MysqlCharset.scala 65.38% <0.00%> (-29.06%) ⬇️
...main/scala/com/twitter/finagle/mysql/Request.scala 66.66% <0.00%> (-18.41%) ⬇️
...twitter/finagle/service/PendingRequestFilter.scala 86.36% <0.00%> (-4.55%) ⬇️
...rverset2/client/apache/ApacheKeeperException.scala 11.11% <0.00%> (-3.71%) ⬇️
.../com/twitter/finagle/tracing/BroadcastTracer.scala 72.91% <0.00%> (ø)
...riftmux/pushsession/MuxDowngradingNegotiator.scala 91.17% <0.00%> (+1.34%) ⬆️
...om/twitter/finagle/dispatch/ServerDispatcher.scala 85.10% <0.00%> (+2.12%) ⬆️
...m/twitter/finagle/exp/ConcurrencyLimitFilter.scala 91.42% <0.00%> (+2.85%) ⬆️
...gle/http2/transport/client/ClientServiceImpl.scala 90.90% <0.00%> (+9.09%) ⬆️
...witter/finagle/mux/pushsession/MessageWriter.scala 87.65% <0.00%> (+9.87%) ⬆️
... and 2 more

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 99ba86b...e9c02b6. Read the comment docs.

@SerCeMan SerCeMan changed the title Upgrade jackson libraries to 2.12.0 Upgrade jackson libraries to 2.12.1 Jan 11, 2021
@yufangong
Copy link
Contributor

yufangong commented Jan 12, 2021

Thank you for the update!
We would like to keep all of our libs on the same version, those are:
util/build.sbt
finagle/build.sbt
twitter-server/build.sbt
finatra/build.sbt

Can you make this PR cover all of those? thanks!

Edit: sorry, I think the ask was impractical, to making a PR across repos. We have an internal ticket scheduled to upgrade the version all across, ty!

@yufangong
Copy link
Contributor

Unfortunately, We have run into a high-risk issue in the internal source: FasterXML/jackson-module-scala#495.
I'm afraid we need to wait until a fix before trying to bump up, the deprecation of ScalaObjectMapper in the patched release is also concerning, so the version update won't be a quick and easy one this time.

@SerCeMan
Copy link
Author

Thanks for the update! Just to clarify, would the above issue cause issues if finagle gets Jackson 2.12 as a transitive dependency? Or is the issue with one of the downstream consumers of Finagle and not Finagle itself?

@yufangong
Copy link
Contributor

Thank you for bringing this up to let it falls on our radar, @SerCeMan. I believe the concern is more for the customers, especially Finatra customers as it provides default JSON serde. Not only we want to keep the ecosystem on the same versions but also the issue is hard to catch without thoroughly detecting this kind of use case: serialize fields in traits and objects vs serialize fields in classes.

@SerCeMan
Copy link
Author

SerCeMan commented Jun 9, 2021

Hey, @yufangong! I'm just checking if the upgrade was unblocked. If I understand correctly, the referenced issue was closed. :)

@yufangong
Copy link
Contributor

@SerCeMan thanks so much for the reminder! We will take give it another try shortly.

@SerCeMan
Copy link
Author

SerCeMan commented Sep 2, 2021

Hey, @yufangong! I'm wondering if there were any new issues with the attempt, I noticed that Jackson is moving to developing the new 2.13 branch now, https://github.com/FasterXML/jackson#active-developed-jackson-2x-branches :)

@tigerlily-he
Copy link
Contributor

Hi @SerCeMan, I work with Yufan at Twitter. Thanks for checking in! We're planning to upgrade Jackson in September and we'll look into Jackson 2.12 and 2.13.

@mosesn
Copy link
Contributor

mosesn commented Nov 9, 2021

@SerCeMan update on this: we upgraded to 2.13.0 b79b1d7 before discovering a regression that doubled the cost of serialization in jackson-databind. We're going to roll back for now, and will roll forward once they release 2.13.1.

@SerCeMan
Copy link
Author

Hey, @mosesn! Thank you for the update!

@tigerlily-he
Copy link
Contributor

Hi @SerCeMan, The January 2022 release of Finagle has the Jackson upgrade to 2.13.1. 831b251.

Thank you for following this journey on the Jackson upgrade! I will close this PR now.

@SerCeMan
Copy link
Author

@tigerlily-he That's awesome, thanks 🎉

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

Successfully merging this pull request may close these issues.

6 participants