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.

Sign up for GitHub

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

Jump to bottom

Update jackson to 2.9.10 #8940

Merged
merged 1 commit into from
Nov 27, 2019
Merged

Update jackson to 2.9.10 #8940

merged 1 commit into from
Nov 27, 2019

Conversation

ccaominh
Copy link
Contributor

Description

Addresses security vulnerabilities:

This PR has:

  • been self-reviewed.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • been tested in a test Druid cluster.

@ccaominh
Update jackson to 2.9.10
bbafbf7 
Addresses security vulnerabilities:

- sonatype-2016-0397:
  FasterXML/jackson-core#315

- sonatype-2017-0355:
  FasterXML/jackson-core#322
@ccaominh ccaominh marked this pull request as ready for review November 26, 2019 20:07
@ccaominh
Copy link
Contributor Author

Also manually tested using https://druid.apache.org/docs/latest/tutorials/tutorial-batch-hadoop.html

@jihoonson
Copy link
Contributor

Would you please add a list of tests you have done?

@ccaominh
Copy link
Contributor Author

In the test cluster, I ran a kinesis ingestion task to check the updated jackson version compatibility with AWS SDK for Java version 1.x.

@jihoonson Are there any additional manual tests that you recommend?

surekhasaharan
surekhasaharan reviewed Nov 26, 2019
View reviewed changes
core/src/test/java/org/apache/druid/timeline/SegmentWithOvershadowedStatusTest.java

// Previously, the implementation of SegmentWithOvershadowedStatus had @JsonCreator/@JsonProperty and @JsonUnwrapped
// on the same field (dataSegment), which used to work in Jackson 2.6, but does not work with Jackson 2.9:
// https://github.com/FasterXML/jackson-databind/issues/265#issuecomment-264344051
Copy link

Choose a reason for hiding this comment

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

that's a bummer :(, should we add a TODO note here to undo the workaround once FasterXML/jackson-databind#1467 is fixed or when/if move to Jackson 3.x ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can add a TODO. I think it'll work well in the implementation class as a reminder to remove the constructor I added.

@ccaominh
Copy link
Contributor Author

Did another manual test that's a variant of https://druid.apache.org/docs/latest/tutorials/tutorial-batch-hadoop.html to have the hadoop index task ingest from s3 and then write to s3 via hdfs.

jihoonson
jihoonson approved these changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

@ccaominh thanks for the tests. The test coverage would be too large to be done by one contributor. I think this PR is now good to go, but we could do more intensive tests before release.

@fjy fjy merged commit fba876b into apache:master Nov 27, 2019
@ccaominh
Copy link
Contributor Author

All the hadoop manual tests were done with hadoop 2.8.5.

@ccaominh ccaominh deleted the update-jackson branch November 27, 2019 19:45
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@surekhasaharan surekhasaharan surekhasaharan left review comments

@jihoonson jihoonson jihoonson approved these changes

Assignees
No one assigned
Labels
Area - Dependencies
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ccaominh @jihoonson @surekhasaharan @fjy @jon-wei