-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HADOOP-19544. Upgrade to Jackson 2.18.5 #8070
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
| <jackson2.version>2.14.3</jackson2.version> | ||
| <jackson2.databind.version>2.14.3</jackson2.databind.version> | ||
| <jackson2.version>2.18.5</jackson2.version> | ||
| <jackson2.databind.version>2.18.5</jackson2.databind.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.
It would be easier to use jackson-bom for version management.
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.
or at least define jackson2.databind.version as ${jackson2.version} until some need to actually diverge surfaces again in future
|
See #8074 for the jackson-bom change. |
steveloughran
left a comment
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.
+1 pending that build.
@stoty I see the BOM PR and think we can consider that separately; merge this in and then look at it.
|
Sure, the BOM change can be done independently either before or after |
|
I don't know what happened to https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-8070/1/ It looks like it failed but I'm not sure what the issue is. |
|
I have restarted the CI job. |
|
💔 -1 overall
This message was automatically generated. |
|
TestDelegatingSSLSocketFactory.testOpenSSL failure unrelated; #8071 fixes it. I just want a complete build there before the merge (it's already approved). |
|
@pjfanning I think the patch is failing compilation can you check once |
|
@ayushtkn looks like this PR is affected by HADOOP-19674 I'll see if it can be fixed |
dependency-convergence issue jackson 2.18.4 Update YarnJacksonJaxbJsonProvider.java Update YarnJacksonJaxbJsonProvider.java jackson 2.18.5
|
I'll reopen this when the solution to https://issues.apache.org/jira/browse/HADOOP-19746 is merged |
4bc142b to
01bf119
Compare
steveloughran
left a comment
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.
What do we do on a backport to branch-3.4 here? same PR is good?
| public ObjectMapper locateMapper(Class<?> type, MediaType mediaType) { | ||
| ObjectMapper mapper = super.locateMapper(type, mediaType); | ||
| configObjectMapper(mapper); | ||
| if (type == TimelineAbout.class) { |
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.
what triggers this? I'm worried about transitive complications.
Is there a jackson issue we could link to in the release notes to warn/explain?
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.
I have no idea what is causing this.
I wouldn't aim to backport this to 3.4. |
steveloughran
left a comment
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.
LGTM
+1
Description of PR
HADOOP-19544 replacing #7623
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?