You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "William1104 (via GitHub)" <gi...@apache.org> on 2024/02/14 15:07:15 UTC
[PR] SPARK-47042 add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
William1104 opened a new pull request, #45101:
URL: https://github.com/apache/spark/pull/45101
### What changes were proposed in this pull request?
This PR aims to fix `spark-common-utils` module by adding explicit `commons-lang3` dependency.
### Why are the changes needed?
`spark-common-utils` uses `commons-lang3` like the following, but we didn't declare it explicitly.
[MavenUtils.scala](https://github.com/William1104/spark/blob/ad93dfb565ad2f6a04505365294bf1b1c603c2ce/common/utils/src/main/scala/org/apache/spark/util/MavenUtils.scala#L25)
[ClosureCleaner.scala]
(https://github.com/William1104/spark/blob/ad93dfb565ad2f6a04505365294bf1b1c603c2ce/common/utils/src/main/scala/org/apache/spark/util/ClosureCleaner.scala#L27)
[JavaUtils.java]
(https://github.com/William1104/spark/blob/ad93dfb565ad2f6a04505365294bf1b1c603c2ce/common/utils/src/main/java/org/apache/spark/network/util/JavaUtils.java#L31)
### How was this patch tested?
Pass the CIs.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47042][BUILD] add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45101:
URL: https://github.com/apache/spark/pull/45101#issuecomment-1944302594
I agree with you at this point.
> By explicitly declaring the dependency, we can avoid any unexpected missing dependencies that might occur when upgrading 'commons-text'.
For the following question, since we cannot enumerate all transitive dependencies always, Apache Spark established a way to depend on an explicit manifest file (of the final Apache Spark distribution) and CI checking.
- https://github.com/apache/spark/blob/master/dev/test-dependencies.sh
- https://github.com/apache/spark/blob/master/dev/deps/spark-deps-hadoop-3-hive-2.3
> . If it is the established practice within the Spark project to address dependency issues
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47042][BUILD] add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45101:
URL: https://github.com/apache/spark/pull/45101#issuecomment-1948933031
Here is a comment about your question.
- https://github.com/apache/spark/pull/45103#issuecomment-1948928099
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47042][BUILD] add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on PR #45101:
URL: https://github.com/apache/spark/pull/45101#issuecomment-1960007867
Likewise i think this is OK. We're not adding a direct dependency because it's a transitive dependency, but because it is used directly in the module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47042][BUILD] add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45101:
URL: https://github.com/apache/spark/pull/45101#issuecomment-1944194924
Please note that we usually fix the wrong dependency like the following situation in order to unblock other PRs. If you have a similar issue, please describe in the PR description, @William1104 .
> Previously, it was provided by some unused `htmlunit-driver`'s transitive dependency accidentally. This causes a weird situation which `kvstore` module starts to fail to compile when we upgrade `htmlunit-driver`. We need to fix this first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47042][BUILD] add missing explicit dependency 'commons-lang3' to the module 'spark-common-utils' [spark]
Posted by "William1104 (via GitHub)" <gi...@apache.org>.
William1104 commented on PR #45101:
URL: https://github.com/apache/spark/pull/45101#issuecomment-1944292205
Hi @dongjoon-hyun
Thank you for taking the time to review my pull request. I appreciate your feedback and would like to address the points you raised.
Regarding the declaration of explicit dependencies, I believe it is a common practice to declare dependencies explicitly when a module contains a class that directly depends on a specific library. This practice ensures that the dependencies are clearly defined and helps mitigate potential issues that may arise in the future. While a library like 'commons-text' may currently have 'commons-lang3' as a transitive dependency, there is no guarantee that this relationship will remain unchanged in future releases. By explicitly declaring the dependency, we can avoid any unexpected missing dependencies that might occur when upgrading 'commons-text'.
However, I understand that different projects may have different approaches to handling dependency issues. If it is the established practice within the Spark project to address dependency issues only when they block enhancements or pose critical problems, I am willing to follow that practice and close this pull request accordingly.
Thank you again for your review, and I look forward to any further guidance you may provide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org