HADOOP-19259. Upgrade to jackson 2.14.3 #7428
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pjfanning Thanks for the contribution! LGTM. This PR contains some code changes, and we need to wait for feedback from Yetus. |
|
💔 -1 overall
This message was automatically generated. |
4 tasks
pjfanning
force-pushed
the
HADOOP-19249-jackson2.14
branch
from
e1abb77 to
99ef4c8
Compare
pjfanning
changed the title
|
@steveloughran are the exception handling changes ok? I brought them over from #7022 |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Feb 24, 2025
There was a problem hiding this comment.
trusting your jackson recommendations here, obviously.
code wise, looks good, the only issue is "should we swallow all exceptions raised during the translation.
Also, I think that could go into NetUtils along with the other exception wrapping there, as it's for others to use, and could then add unit tests
- one for (string, throwable
- something that takes a (string),
- one whose ctor is ()
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/web/WebHdfsFileSystem.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
pan3793
reviewed
Feb 25, 2025
hadoop-project/pom.xml
Outdated
| <exclusion> | ||
| <groupId>javax.xml.bind</groupId> | ||
| <artifactId>jaxb-api</artifactId> | ||
| </exclusion> |
There was a problem hiding this comment.
why this exclusion is required?
There was a problem hiding this comment.
Hadoop loads this anyway. The issue is that the dependency version convergence checks fail the build if transitive dependency versions to not match the explicit dependency version.
There was a problem hiding this comment.
but I didn't see the excluded dep in the list
https://mvnrepository.com/artifact/com.fasterxml.jackson.module/jackson-module-jaxb-annotations/2.14.3
do you mean jakarta.xml.bind:jakarta.xml.bind-api?
There was a problem hiding this comment.
Have a look at https://github.com/search?q=repo%3Aapache%2Fhadoop%20javax.xml.bind&type=code
javax.xml.bind is still used in Hadoop.
I did not add this exclusion on a whim. I added it because the Haddop build failed with a dreaded dependency convergence exception.
The tests pass with this change.
There was a problem hiding this comment.
I'll remove the exclusion and see if the dependency convergence exception still occurs. You are right that jackson-module-jaxb-annotations:2.14.3 depends on the jakarta variant of the jar.
There was a problem hiding this comment.
I added the exclusion when testing with jackson 2.18.2.
that has a dependency on javax.xml.bind:jaxb-api not jakarta.xml.bind:jakarta.xml.bind-api
jakarta.xml.bind:jakarta.xml.bind-api v2 uses the same package name as javax.xml.bind:jaxb-api (a javax package name) - it was only in v3 that the package name changed to a jakarta package name.
I think I will add back the exclusion but fix it so that it excludes jakarta.xml.bind:jakarta.xml.bind-api.
pan3793
reviewed
Feb 26, 2025
| @@ -504,8 +504,7 @@ javax.cache:cache-api:1.1.1 | |||
| javax.servlet:javax.servlet-api:3.1.0 | |||
| javax.servlet.jsp:jsp-api:2.1 | |||
| javax.websocket:javax.websocket-api:1.0 | |||
| javax.ws.rs:jsr311-api:1.1.1 | |||
There was a problem hiding this comment.
dependency was removed during the move to jersey v2 - we now have rs-api dependency instead
There was a problem hiding this comment.
Alright, JavaEE/JakartaEE API dependencies are a mess...
|
💔 -1 overall
This message was automatically generated. |
pan3793
approved these changes
Feb 26, 2025
steveloughran
requested changes
Feb 27, 2025
| @Test | ||
| public void testAddNodeNameToIOException() { | ||
| IOException e0 = new IOException("test123"); | ||
| assertNull(e0.getCause()); |
There was a problem hiding this comment.
can we have assertj here..as we do that move to junit5, best to start off with the cross-version test asserts.
actually, given there's so many assertNull(throwable.getCause(), what about a new method assertNullCause(Throwable) and use through this
|
|
||
| IOException e1 = new IOException("test456", new IllegalStateException("deliberate")); | ||
| IOException new1 = NetUtils.addNodeNameToIOException(e1, "node456"); | ||
| assertEquals(e1.getCause(), new1.getCause()); |
There was a problem hiding this comment.
assertEquals or assertSame?
TestNetUtils already is junit 5 (jupiter) |
|
💔 -1 overall
This message was automatically generated. |
pjfanning
force-pushed
the
HADOOP-19249-jackson2.14
branch
from
5833fcc to
4db0263
Compare
|
💔 -1 overall
This message was automatically generated. |
try to make exception manipulation more robust jackson 2.18.2 jaxb convergence issue try to use DynConstructors try to use DynConstructors issue with exceptions that can no longer be thrown jackson 2.14.3
pjfanning
force-pushed
the
HADOOP-19249-jackson2.14
branch
from
4db0263 to
782487e
Compare
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Hitting issues in jackson 2.18 and there is a bug in jackson 2.18.2 that I want to avoid. I think it is useful to upgrade to jackson 2.14.3 first. It has some security hardening and bug fixes but avoids some major changes in jackson 2.15+.
Replacement for #7022
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?