[SPARK-40635][YARN][TESTS] Fix yarn module daily test failed with hadoop2
#38079
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Failed as follows: |
LuciferYang
commented
Oct 3, 2022
.github/workflows/build_and_test.yml
Outdated
| required: false | ||
| type: string | ||
| default: hadoop3 | ||
| default: hadoop2 |
Contributor
Author
There was a problem hiding this comment.
for test hadoop-2, will revert after confirmation takes effect
Contributor
Author
|
Manual test ：
locally, but they all passed. From the characteristics of the failed case, the am start failed because the start command line is too long maybe due to the classpath contains too many jars(CLASSPATH and SPARK_DIST_CLASSPATH includes all jars in .cache) |
Contributor
Author
|
Member
|
Thanks for investigating this @LuciferYang. cc @Yikun FYI |
Contributor
Author
|
https://github.com/LuciferYang/spark/actions/runs/3171106629/jobs/5164233303
It seems that my guess is correct. It is effective to manually filter some unnecessary jars from classpath to reduce the length of the start command, such as So I have the following two ideas:
spark/project/SparkBuild.scala Lines 917 to 922 in 0870822 @HyukjinKwon @Yikun Do you have any other suggestions？ In addition, I found that the prefix of a large number of jar packages under the classpath is |
Contributor
Author
|
The submit job |
LuciferYang
commented
Oct 3, 2022
| addClasspathEntry(getClusterPath(sparkConf, cp), env) | ||
| val newCp = cp.split(CLASS_PATH_SEPARATOR) | ||
| .filterNot(cpSet.contains).mkString(CLASS_PATH_SEPARATOR) | ||
| addClasspathEntry(getClusterPath(sparkConf, newCp), env) |
Contributor
Author
There was a problem hiding this comment.
Try to deduplicate jars that have been added to CLASSPATH through extraClassPath
Contributor
Author
There was a problem hiding this comment.
Is it necessary to change it to
val newCp = if (Utils.isTesting) {
cp.split(File.pathSeparator)
.filterNot(cpSet.contains).mkString(File.pathSeparator)
} else cp
but I think de duplication may also be useful for the production environment
Contributor
Author
|
64f0e0b seems work normal |
LuciferYang
changed the title
Scala 2.12 + Hadoop 2 + JDK 8 daily testScala 2.12 + Hadoop 2 + JDK 8 daily test
LuciferYang
changed the title
Scala 2.12 + Hadoop 2 + JDK 8 daily testyarn module daily test failed with hadoop2
HyukjinKwon
reviewed
Oct 5, 2022
| } | ||
|
|
||
| val cpSet = extraClassPath match { | ||
| case Some(classPath) => classPath.split(File.pathSeparator).toSet |
Member
There was a problem hiding this comment.
Actually this is risky because it loses the order of classpaths. This matters when multiple jars are provided somehow in the production env. Should use sth like LinkedHashSet (see also HyukjinKwon@c6b8eb7)
Member
There was a problem hiding this comment.
Oh, nvm. okay, it is just used for checking if sth exists. All good.
Member
There was a problem hiding this comment.
I would use getOrElse approach tho.
Contributor
Author
There was a problem hiding this comment.
Do you mean extraClassPath.getOrElse("").split(File.pathSeparator).toSet?
How about
val cpSet = extraClassPath match {
case Some(classPath) if Utils.isTesting =>
classPath.split(File.pathSeparator).toSet
case _ => Set.empty[String]
}
Contributor
Author
There was a problem hiding this comment.
If only for testing, we can do split only when Utils.isTesting is true
HyukjinKwon
reviewed
Oct 5, 2022
| sys.env.get(ENV_DIST_CLASSPATH).foreach { cp => | ||
| addClasspathEntry(getClusterPath(sparkConf, cp), env) | ||
| val newCp = cp.split(File.pathSeparator) | ||
| .filterNot(cpSet.contains).mkString(File.pathSeparator) |
Member
There was a problem hiding this comment.
I don't think this is still safe because of classpath order. Let's do this for test only.
Contributor
Author
There was a problem hiding this comment.
Ok, hmm... but will /rootpath/subdir/a-1.0.jar:/rootpath/subdir/b-1.0.jar:/rootpath/subdir/a-1.0.jar and /rootpath/subdir/a-1.0.jar:/rootpath/subdir/b-1.0.jar have different results?
The de-duplication operation is a full path matching
Contributor
Author
HyukjinKwon
reviewed
Oct 5, 2022
|
|
||
| sys.env.get(ENV_DIST_CLASSPATH).foreach { cp => | ||
| addClasspathEntry(getClusterPath(sparkConf, cp), env) | ||
| val newCp = if (Utils.isTesting) { |
Member
There was a problem hiding this comment.
Maybe let's add a comment with mentioning the JIRA number.
Contributor
Author
There was a problem hiding this comment.
ok, will add comments after the hadoop2 check pass
Contributor
Author
Contributor
Author
|
2712ae0 add comments and check hadoop3 |
Member
|
Merged to master. |
Contributor
Author
|
thanks @HyukjinKwon |
Contributor
Author
|
https://github.com/apache/spark/actions/runs/3190113779 Yeah ~ build success |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This pr adds the de duplication process in the
yarn.Client#populateClasspathmethod whenUtils.isTestingis true to ensure thatENV_DIST_CLASSPATHwill only add the part ofextraClassPaththat does not exist toCLASSPATHto avoidjava.io.IOException: error=7, Argument list too longin this way.Why are the changes needed?
Fix daily test failed of yarn module with
-Phadoop-2.Daily test failed as follows:
Does this PR introduce any user-facing change?
No
How was this patch tested?
hadoop2is successful: https://github.com/LuciferYang/spark/actions/runs/3175111616/jobs/5172833416