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

[#1699] improvement: Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections:4.4 #1700

Merged
merged 3 commits into from
May 17, 2024

Conversation

rickyma
Copy link
Contributor

@rickyma rickyma commented May 11, 2024

What changes were proposed in this pull request?

Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections:4.4.

Why are the changes needed?

Fix: #1699.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

@rickyma rickyma changed the title [#1699] improvement: Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections4.5.0-M1 [#1699] improvement: Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections:4.5.0-M1 May 11, 2024
Copy link

github-actions bot commented May 11, 2024

Test Results

 2 405 files  +14   2 405 suites  +14   4h 58m 14s ⏱️ + 1m 24s
   931 tests + 3     930 ✅ + 3   1 💤 ±0  0 ❌ ±0 
10 791 runs  +37  10 777 ✅ +37  14 💤 ±0  0 ❌ ±0 

Results for commit 0db25f4. ± Comparison against base commit 8e26a34.

♻️ This comment has been updated with latest results.

…ollections:3.2.2 to org.apache.commons:commons-collections4.5.0-M1
@rickyma
Copy link
Contributor Author

rickyma commented May 16, 2024

PTAL. @jerqi @zuston

@zuston
Copy link
Member

zuston commented May 16, 2024

Is this common update for other opensource projects?

@rickyma
Copy link
Contributor Author

rickyma commented May 16, 2024

I think it is. You can refer to https://issues.apache.org/jira/browse/SPARK-37968.

@jerqi
Copy link
Contributor

jerqi commented May 16, 2024

@LuciferYang Could you help me review this pull request?

@jerqi jerqi requested a review from LuciferYang May 16, 2024 06:26
pom.xml Outdated
@@ -56,7 +56,7 @@
<version.checksum-maven-plugin>1.11</version.checksum-maven-plugin>
<awaitility.version>4.2.0</awaitility.version>
<checkstyle.version>9.3</checkstyle.version>
<commons-collections.version>3.2.2</commons-collections.version>
<commons-collections4.version>4.5.0-M1</commons-collections4.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest migrating to 4.4 first, rather than a milestone version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@rickyma rickyma changed the title [#1699] improvement: Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections:4.5.0-M1 [#1699] improvement: Upgrade from commons-collections:commons-collections:3.2.2 to org.apache.commons:commons-collections:4.4 May 16, 2024
@rickyma rickyma requested a review from LuciferYang May 16, 2024 06:59
<groupId>commons-collections</groupId>
<artifactId>commons-collections</artifactId>
<version>${commons-collections.version}</version>
<groupId>org.apache.commons</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

should only modify the commons-collections part, no need to touch the commons-lang3 part

Copy link
Contributor

Choose a reason for hiding this comment

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

Others is fine to me. But please check whether there are cascading dependencies on commons-collections 3. If there are, whether it is necessary to retain the definition of commons-collections 3 version in uniffle. @rickyma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed the order of the dependencies. commons-lang3 remains the same as before.

The UTs have all passed, so the casading dependencies should be OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

The UTs have all passed, so the casading dependencies should be OK.

OK. It would be even better if we could double check this through the dependency tree.

I just changed the order of the dependencies. commons-lang3 remains the same as before.

Can we maintain the original order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can find commons-collections 3 in the dependency tree which is introduced by hadoop-common, but it is provided:

[INFO] +- org.apache.hadoop:hadoop-common:jar:2.8.5:provided
[INFO] |  +- commons-collections:commons-collections:jar:3.2.2:provided

@LuciferYang

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@LuciferYang LuciferYang merged commit 2085e42 into apache:master May 17, 2024
41 checks passed
@LuciferYang
Copy link
Contributor

LuciferYang commented May 17, 2024

Merged into master. Thanks @rickyma

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants