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

reverting jackson version bump for sql #2978

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

jerrypeng
Copy link
Contributor

Motivation

Bumping Jackson version to 2.9.7 breaks the Presto pulsar connector since the presto version we are using packages Jackson 2.8.1 via Airlift.

In the latest version of Airlift (version 88), Jackson 2.8.1 is packaged but presto is yet to release a version with the latest Airlift version. We will have to wait for a presto version that includes that version or later of Airlift.

I also added additional checks to presto pulsar integration test that covers that functionality that was broken because of this issue

@jerrypeng jerrypeng requested review from sijie, rdhabalia, merlimat and srkukarni and removed request for sijie, rdhabalia, merlimat and srkukarni November 12, 2018 22:47
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@merlimat
Copy link
Contributor

@jerrypeng this doesn't need to be backported to 2.2.1, right?

@merlimat merlimat added this to the 2.3.0 milestone Nov 12, 2018
@merlimat merlimat added area/build area/sql Pulsar SQL related features labels Nov 12, 2018
@jerrypeng
Copy link
Contributor Author

@merlimat yes

Copy link
Contributor

@rdhabalia rdhabalia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm.. instead reverting, can't we make fix in presto distribution because that's what we want ultimately. I think jackson-databind upgrade is somewhat required due to vulnerability issue and multiple org including Oath enforces to use fixed version.
if you really don't want to fix it in presto right now, then let's upgrade to 2.8.11.1 which also has the fix.

@srkukarni
Copy link
Contributor

Agreed with @rdhabalia.

@jerrypeng
Copy link
Contributor Author

jerrypeng commented Nov 13, 2018

@rdhabalia sounds good, I have changed the jackson databind dependency to be 2.8.11.1 to include the fix for the vulnerability. The current presto version we use is tested with jackson 2.8.x.x so I would rather keep that version for now until presto releases a version using jackson 2.9.x.x

<jackson.version>2.8.11</jackson.version>
<!--fix Security Vulnerabilities-->
<!--https://www.cvedetails.com/vulnerability-list/vendor_id-15866/product_id-42991/Fasterxml-Jackson-databind.html-->
<jackson.databind.version>2.8.11.1</jackson.databind.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we use jackson.version for all jackson dependencies ? I don't think there will be any issue because 2.8.11.1 has only data-bind change on top of 2.8.11. So, we might not need two separate versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdhabalia there is only 2.8.11.1 for databind, there isn't that version for other jackson dependencies that aren't effect by this vulnerability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. I see.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdhabalia This is just for presto, the rest of our modules are still with 2.9.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build area/sql Pulsar SQL related features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants