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

Upgrade hadoop version to 3.4.0 #13582

Merged
merged 5 commits into from
Jul 12, 2024
Merged

Upgrade hadoop version to 3.4.0 #13582

merged 5 commits into from
Jul 12, 2024

Conversation

abhioncbr
Copy link
Contributor

Labels

  • dependencies

Description
As per the issue, This PR upgrades the Hadoop version to 3.4.0. There was the jetbrains.annotations dependency convergence issue, and to fix it, we are explicitly importing the dependency in the parent pom file.

@abhioncbr abhioncbr requested a review from Jackie-Jiang July 11, 2024 00:19
@abhioncbr abhioncbr added the dependencies Pull requests that update a dependency file label Jul 11, 2024
Jackie-Jiang
Jackie-Jiang reviewed Jul 11, 2024
View reviewed changes
pom.xml Outdated
@@ -234,6 +234,7 @@

<!-- Solve conflict across dependencies -->
<kotlin.stdlib.version>1.9.24</kotlin.stdlib.version>
<jetbrains.annotations.version>13.0</jetbrains.annotations.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try using the latest version (24.1.0) of this library and see if it works

Jackie-Jiang
Jackie-Jiang reviewed Jul 11, 2024
View reviewed changes
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Seems the dependency conflict is from kotlin-stdlib and orc-core. Wondering why it is not triggered before? Neither of them seems related to the hadoop version bump?

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.11%. Comparing base (59551e4) to head (dd7863c).
Report is 747 commits behind head on master.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #13582      +/-   ##
============================================
+ Coverage     61.75%   62.11%   +0.36%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2558     +122     
  Lines        133233   140968    +7735     
  Branches      20636    21869    +1233     
============================================
+ Hits          82274    87569    +5295     
- Misses        44911    46767    +1856     
- Partials       6048     6632     +584
Flag Coverage Δ
custom-integration1 <0.01% <ø> (-0.01%) ⬇️
integration <0.01% <ø> (-0.01%) ⬇️
integration1 <0.01% <ø> (-0.01%) ⬇️
integration2 0.00% <ø> (ø)
java-11 62.04% <ø> (+0.33%) ⬆️
java-21 61.98% <ø> (+0.36%) ⬆️
skip-bytebuffers-false 62.09% <ø> (+0.34%) ⬆️
skip-bytebuffers-true 61.94% <ø> (+34.21%) ⬆️
temurin 62.11% <ø> (+0.36%) ⬆️
unittests 62.11% <ø> (+0.36%) ⬆️
unittests1 46.68% <ø> (-0.21%) ⬇️
unittests2 27.67% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abhioncbr
Copy link
Contributor Author

Seems the dependency conflict is from kotlin-stdlib and orc-core. Wondering why it is not triggered before? Neither of them seems related to the hadoop version bump?

This is strange for me as well. I will be digging more into it. Thanks.

@abhioncbr abhioncbr force-pushed the hadoop-version-upgrade-3.4.0 branch from e29c818 to 1f13af7 Compare July 12, 2024 00:06
@abhioncbr
Copy link
Contributor Author

Seems the dependency conflict is from kotlin-stdlib and orc-core. Wondering why it is not triggered before? Neither of them seems related to the hadoop version bump?

This is strange for me as well. I will be digging more into it. Thanks.

I think the reason is that in hadoop 3.3.6 they have the compile dependency on kotlin-stdlib, which is not the case in hadoop 3.4.0

[INFO] +- org.apache.hadoop:hadoop-hdfs-client:jar:3.3.6:compile
[INFO] |  +- com.squareup.okhttp3:okhttp:jar:4.9.3:compile
[INFO] |  |  \- com.squareup.okio:okio:jar:3.9.0:compile
[INFO] |  |     \- com.squareup.okio:okio-jvm:jar:3.9.0:compile
[INFO] |  +- org.jetbrains.kotlin:kotlin-stdlib:jar:2.0.0:compile
[INFO] |  +- org.jetbrains.kotlin:kotlin-stdlib-common:jar:2.0.0:compile


[INFO] +- org.apache.hadoop:hadoop-hdfs-client:jar:3.4.0:compile
[INFO] |  \- com.fasterxml.jackson.core:jackson-annotations:jar:2.17.2:compile

Jackie-Jiang
Jackie-Jiang approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

pinot-plugins/pinot-input-format/pinot-orc/pom.xml
Comment on lines +77 to +80
<dependency>
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java</artifactId>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Without it, we will get the exception com.google.protobuf.MessageOrBuilder not found

@Jackie-Jiang Jackie-Jiang merged commit aeaf7cf into apache:master Jul 12, 2024
22 checks passed
rajagopr pushed a commit to rajagopr/pinot that referenced this pull request Jul 17, 2024
@abhioncbr @rajagopr
Upgrade hadoop version to 3.4.0 (apache#13582)
bfd13ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jackie-Jiang Jackie-Jiang Jackie-Jiang approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abhioncbr @codecov-commenter @Jackie-Jiang