-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HADOOP-16908. Prune Jackson 1 from the codebase and restrict it's usage for future #3789
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not all javac warnings are applicable to this change. For instance, this warning is not relevant: |
Just to provide bit more context, mvn dependency shows that only avro 1.7.7 needs codehaus jackson dependency. From Jersey 1.x, we are excluding codehaus jackson explicitly. IMHO, we can upgrade avro to the version not using codehaus jackson in a separate Jira once this PR merged. Also, this PR is anyways going to ensure that codehaus jackson is never used in the codebase again. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
<exclusions> | ||
<exclusion> | ||
<groupId>org.codehaus.jackson</groupId> | ||
<artifactId>jackson-core-asl</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>org.codehaus.jackson</groupId> | ||
<artifactId>jackson-mapper-asl</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>org.codehaus.jackson</groupId> | ||
<artifactId>jackson-jaxrs</artifactId> | ||
</exclusion> | ||
<exclusion> | ||
<groupId>org.codehaus.jackson</groupId> | ||
<artifactId>jackson-xc</artifactId> | ||
</exclusion> | ||
</exclusions> |
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.
This is required to avoid dependency convergence issue:
Dependency convergence error for org.codehaus.jackson:jackson-mapper-asl:1.9.2 paths to dependency are:
+-org.apache.hadoop:hadoop-common:3.4.0-SNAPSHOT
+-com.sun.jersey:jersey-json:1.19
+-org.codehaus.jackson:jackson-mapper-asl:1.9.2
and
+-org.apache.hadoop:hadoop-common:3.4.0-SNAPSHOT
+-com.sun.jersey:jersey-json:1.19
+-org.codehaus.jackson:jackson-jaxrs:1.9.2
+-org.codehaus.jackson:jackson-mapper-asl:1.9.2
and
+-org.apache.hadoop:hadoop-common:3.4.0-SNAPSHOT
+-com.sun.jersey:jersey-json:1.19
+-org.codehaus.jackson:jackson-xc:1.9.2
+-org.codehaus.jackson:jackson-mapper-asl:1.9.2
and
+-org.apache.hadoop:hadoop-common:3.4.0-SNAPSHOT
+-org.apache.avro:avro:1.7.7
+-org.codehaus.jackson:jackson-mapper-asl:1.9.13
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.
Thank you @virajjasani for cleaning up! Added some comments:
.../src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/ProvidedVolumeImpl.java
Outdated
Show resolved
Hide resolved
465f4d9
to
aab4cd1
Compare
I believe it does make sense to keep Jackson1 in the dependency given that it's still not completely removed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@aajisaka The spotbug warning is relevant to the
Does it sound good to you? |
I just realized that the above class |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Sounds good. Thank you @virajjasani |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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. Thank you @virajjasani
…ge for future (apache#3789) Signed-off-by: Akira Ajisaka <[email protected]>
…ge for future (apache#3789) Signed-off-by: Akira Ajisaka <[email protected]>
…ge for future (apache#3789) Signed-off-by: Akira Ajisaka <[email protected]>
Description of PR
Prune Jackson1 usages from entire Hadoop codebase. Restrict the imports from org.codehaus.jackson.
mvn dependency shows that only avro 1.7.7 needs codehaus jackson dependency. From Jersey 1.x, we are excluding codehaus jackson explicitly.
We can upgrade avro to the version not using codehaus jackson in a separate Jira once this PR merged. Also, this PR is anyways going to ensure that codehaus jackson is never used in the codebase again.
How was this patch tested?
Local testing. Full QA results are also available from Jenkins builds.
For code changes: