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

HADOOP-16908. Prune Jackson 1 from the codebase and restrict it's usage for future #3789

Merged
merged 5 commits into from
Dec 20, 2021

Conversation

virajjasani
Copy link
Contributor

@virajjasani virajjasani commented Dec 11, 2021

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:

  • Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?

@virajjasani virajjasani changed the title HADOOP-16908. Prune Jackson 1 HADOOP-16908. Prune Jackson 1 from the codebase and restrict it's usage for future Dec 12, 2021
@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@virajjasani
Copy link
Contributor Author

Not all javac warnings are applicable to this change. For instance, this warning is not relevant:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/util/timeline/TimelineEntityV2Converter.java:115:20:[unchecked] unchecked cast

@virajjasani
Copy link
Contributor Author

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.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

LICENSE-binary Outdated Show resolved Hide resolved
Comment on lines +150 to +167
<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>
Copy link
Contributor Author

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

Copy link
Member

@aajisaka aajisaka left a 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:

LICENSE-binary Outdated Show resolved Hide resolved
hadoop-project/pom.xml Outdated Show resolved Hide resolved
@virajjasani
Copy link
Contributor Author

I believe it does make sense to keep Jackson1 in the dependency given that it's still not completely removed.
Addressed the review comments @aajisaka. Thanks

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@virajjasani
Copy link
Contributor Author

@aajisaka The spotbug warning is relevant to the READER that we removed in the recent change and according to spotbugs, we can remove one more boolean param atEnd from static class ProvidedBlockIteratorState. This field is already annotated with @JsonProperty, hence I believe it's removal can be a separate task if required and we might not need to take it up with this PR.

    @JsonProperty
    private boolean atEnd;

Does it sound good to you?

@virajjasani
Copy link
Contributor Author

I just realized that the above class ProvidedBlockIteratorState is only used by ObjectWriter to print the object properties in the log. We are not deserializing this object ever, hence we can remove above atEnd property as well. Let me add the addendum.

@hadoop-yetus

This comment has been minimized.

@hadoop-yetus

This comment has been minimized.

@aajisaka
Copy link
Member

Sounds good. Thank you @virajjasani

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 40s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 8 new or modified test files.
_ trunk Compile Tests _
+0 🆗 mvndep 13m 9s Maven dependency ordering for branch
+1 💚 mvninstall 21m 46s trunk passed
+1 💚 compile 22m 18s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 compile 19m 24s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 checkstyle 3m 39s trunk passed
+1 💚 mvnsite 25m 49s trunk passed
+1 💚 javadoc 7m 55s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 7m 59s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗 spotbugs 0m 20s branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚 shadedclient 54m 29s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 40s Maven dependency ordering for patch
+1 💚 mvninstall 30m 28s the patch passed
+1 💚 compile 21m 42s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javac 21m 42s the patch passed
+1 💚 compile 19m 24s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 javac 19m 24s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 3m 39s root: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
+1 💚 mvnsite 21m 7s the patch passed
+1 💚 xml 0m 13s The patch has no ill-formed XML file.
+1 💚 javadoc 7m 51s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 8m 2s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗 spotbugs 0m 21s hadoop-project has no data from spotbugs
+1 💚 shadedclient 52m 13s patch has no errors when building and testing our client artifacts.
_ Other Tests _
-1 ❌ unit 774m 15s /patch-unit-root.txt root in the patch passed.
+1 💚 asflicense 1m 37s The patch does not generate ASF License warnings.
1162m 11s
Reason Tests
Failed junit tests hadoop.tools.dynamometer.TestDynamometerInfra
hadoop.yarn.csi.client.TestCsiClient
hadoop.hdfs.server.federation.router.TestRouterRPCMultipleDestinationMountTableResolver
hadoop.ipc.TestIPC
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/27/artifact/out/Dockerfile
GITHUB PR #3789
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient codespell xml spotbugs checkstyle
uname Linux 71a615cc9b4a 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / d8034ef
Default Java Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/27/testReport/
Max. process+thread count 3865 (vs. ulimit of 5500)
modules C: hadoop-project hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager hadoop-hdfs-project/hadoop-hdfs-rbf hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-documentstore hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-applications-catalog/hadoop-yarn-applications-catalog-webapp hadoop-tools/hadoop-dynamometer/hadoop-dynamometer-infra hadoop-tools/hadoop-azure hadoop-tools/hadoop-sls hadoop-tools/hadoop-resourceestimator . U: .
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/27/console
versions git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 38s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 1s No case conflicting files found.
+0 🆗 codespell 0m 0s codespell was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
+1 💚 test4tests 0m 0s The patch appears to include 8 new or modified test files.
_ trunk Compile Tests _
+0 🆗 mvndep 13m 22s Maven dependency ordering for branch
+1 💚 mvninstall 21m 40s trunk passed
+1 💚 compile 22m 10s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 compile 19m 17s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 checkstyle 3m 37s trunk passed
+1 💚 mvnsite 25m 48s trunk passed
+1 💚 javadoc 7m 54s trunk passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 7m 59s trunk passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗 spotbugs 0m 20s branch/hadoop-project no spotbugs output file (spotbugsXml.xml)
+1 💚 shadedclient 52m 0s branch has no errors when building and testing our client artifacts.
_ Patch Compile Tests _
+0 🆗 mvndep 0m 40s Maven dependency ordering for patch
+1 💚 mvninstall 30m 8s the patch passed
+1 💚 compile 21m 36s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javac 21m 36s the patch passed
+1 💚 compile 19m 22s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+1 💚 javac 19m 22s the patch passed
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 checkstyle 3m 31s root: The patch generated 0 new + 26 unchanged - 1 fixed = 26 total (was 27)
+1 💚 mvnsite 21m 11s the patch passed
+1 💚 xml 0m 13s The patch has no ill-formed XML file.
+1 💚 javadoc 7m 55s the patch passed with JDK Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04
+1 💚 javadoc 8m 9s the patch passed with JDK Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
+0 🆗 spotbugs 0m 21s hadoop-project has no data from spotbugs
+1 💚 shadedclient 52m 18s patch has no errors when building and testing our client artifacts.
_ Other Tests _
-1 ❌ unit 768m 48s /patch-unit-root.txt root in the patch passed.
+1 💚 asflicense 1m 34s The patch does not generate ASF License warnings.
1153m 36s
Reason Tests
Failed junit tests hadoop.yarn.csi.client.TestCsiClient
hadoop.tools.dynamometer.TestDynamometerInfra
hadoop.hdfs.server.federation.router.TestRouterRPCMultipleDestinationMountTableResolver
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/28/artifact/out/Dockerfile
GITHUB PR #3789
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient codespell xml spotbugs checkstyle
uname Linux dca62f377ef6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/bin/hadoop.sh
git revision trunk / d8034ef
Default Java Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.11+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_292-8u292-b10-0ubuntu1~20.04-b10
Test Results https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/28/testReport/
Max. process+thread count 3421 (vs. ulimit of 5500)
modules C: hadoop-project hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager hadoop-hdfs-project/hadoop-hdfs-rbf hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-timelineservice-documentstore hadoop-yarn-project/hadoop-yarn/hadoop-yarn-applications/hadoop-yarn-applications-catalog/hadoop-yarn-applications-catalog-webapp hadoop-tools/hadoop-dynamometer/hadoop-dynamometer-infra hadoop-tools/hadoop-azure hadoop-tools/hadoop-sls hadoop-tools/hadoop-resourceestimator . U: .
Console output https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-3789/28/console
versions git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.14.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

Copy link
Member

@aajisaka aajisaka left a 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

@aajisaka aajisaka merged commit 04b6b9a into apache:trunk Dec 20, 2021
ashutoshcipher pushed a commit to ashutoshcipher/hadoop that referenced this pull request Dec 22, 2021
HarshitGupta11 pushed a commit to HarshitGupta11/hadoop that referenced this pull request Nov 28, 2022
ian-j-abbott-accenture pushed a commit to UrbanOS-Public/urbanos-hadoop that referenced this pull request Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants