[SPARK-18099][YARN] Fail if same files added to distributed cache for --files and --archives by kishorvpatil · Pull Request #15627 · apache/spark

kishorvpatil · 2016-10-25T18:24:12Z

What changes were proposed in this pull request?

During spark-submit, if yarn dist cache is instructed to add same file under --files and --archives, This code change ensures the spark yarn distributed cache behaviour is retained i.e. to warn and fail if same files is mentioned in both --files and --archives.

How was this patch tested?

Manually tested:

if same jar is mentioned in --jars and --files it will continue to submit the job.

basically functionality [SPARK-14423] [SPARK-14423][YARN] Avoid same name files added to distributed cache again #12203 is unchanged
1. if same file is mentioned in --files and --archives it will fail to submit the job.

Please review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening a pull request.

… under archives and files

SparkQA · 2016-10-25T18:53:02Z

Test build #67525 has finished for PR 15627 at commit 9bb1623.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-10-26T14:02:55Z

@kishorvpatil please look at the test failure

SparkQA · 2016-10-26T21:10:22Z

Test build #67600 has finished for PR 15627 at commit a3eb6d4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

… under archives and files

tgravescs · 2016-10-27T15:17:03Z

Jenkins, test this please

tgravescs · 2016-10-27T15:17:32Z

Jenkins, add to whitelist

tgravescs · 2016-10-27T15:31:06Z

+ cachedSecondaryJarLinks += localizedPath
+ }
+ } else {
+ require(localizedPath !=null)


add space after !=

Lets change the error to illegal argument exception.
Also lets comment this to indicate jars are ok due to spark 2.0 jar install, everything else shouldn't have multiple of same jar/file/archive.

SparkQA · 2016-10-27T15:42:23Z

Test build #67648 has finished for PR 15627 at commit 2c55fc2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-27T15:48:02Z

Test build #67649 has finished for PR 15627 at commit 2c55fc2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-10-28T19:42:28Z

+ } else {
+ if (localizedPath != null) {
+ throw new IllegalArgumentException(s"Attempt to add ($file) multiple times. " +
+ "Please check the values of 'spark.yarn.dist.files' and/or " +


can you remove the part about check values of those specific configs because there are multiple ways for these to specified (configs or --files, --jars, etc). Perhaps just say please check the values you specified for uploading files,jars, and archives to make sure one isn't specified multiple times..

SparkQA · 2016-10-28T19:59:06Z

Test build #67719 has finished for PR 15627 at commit a1dc858.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-28T20:17:47Z

Test build #67720 has finished for PR 15627 at commit 33f95ab.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-10-28T20:38:43Z

 val (_, localizedPath) = distribute(file, resType = resType)
- if (addToClasspath && localizedPath != null) {
- cachedSecondaryJarLinks += localizedPath
+ if (addToClasspath) {


can you add a comment here explaining what exactly thi sis doing to help explain and keep from breaking in future.
Also can you add another unit test to cover this case.

SparkQA · 2016-10-31T19:35:17Z

Test build #67826 has finished for PR 15627 at commit f797481.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-11-01T15:14:21Z

+ val userLib1 = Utils.createTempDir()
+ val userLib2 = Utils.createTempDir()
+
+ val jar1 = TestUtils.createJarWithFiles(Map(), jarsDir)


not used anywhere

tgravescs · 2016-11-01T15:15:46Z

+ test("distribute archive multiple times") {
+ val libs = Utils.createTempDir()
+ val jarsDir = new File(libs, "jars")
+ assert(jarsDir.mkdir())


I don't see jarsDir being used anywhere either

tgravescs · 2016-11-01T15:25:54Z

+ val output = new FileOutputStream(target)
+ Utils.copyStream(input, output, closeStreams = true)
+ target.toURI.toURL
+ }


If we can cleanup the variables names above I think it would help a lot, the test is confusing. I know you just copy and pasted but would be nice to clean up.
Also can we have 3 tests or 3 asserts,
one for same file in --files
one for same file in --archives
one for same file in --files and --archives

SparkQA · 2016-11-03T20:57:10Z

Test build #68083 has finished for PR 15627 at commit 51eefa5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2016-11-03T21:07:42Z

+1

… --files and --archives ## What changes were proposed in this pull request? During spark-submit, if yarn dist cache is instructed to add same file under --files and --archives, This code change ensures the spark yarn distributed cache behaviour is retained i.e. to warn and fail if same files is mentioned in both --files and --archives. ## How was this patch tested? Manually tested: 1. if same jar is mentioned in --jars and --files it will continue to submit the job. - basically functionality [SPARK-14423] #12203 is unchanged 1. if same file is mentioned in --files and --archives it will fail to submit the job. Please review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening a pull request. … under archives and files Author: Kishor Patil <kpatil@yahoo-inc.com> Closes#15627 from kishorvpatil/spark18099. (cherry picked from commit 098e4ca) Signed-off-by: Tom Graves <tgraves@yahoo-inc.com>

ueshin · 2016-11-08T04:42:13Z

@kishorvpatil @tgravescs It seems this pr is breaking functionalities of --files or --archives.
Using --files or --archives with files which are not included to --jars doesn't work.

ueshin · 2016-11-08T06:28:54Z

 }
 } else {
- require(localizedPath !=null)
+ if (localizedPath != null) {


I guess here is localizedPath == null ?

tgravescs · 2016-11-08T15:39:53Z

thanks for pointing this out

tgravescs · 2016-11-08T15:45:18Z

SPARK-18357 filed to fix

kishorvpatil · 2016-11-08T17:23:57Z

@ueshin Sorry about this. The patch is available in #15810.

## What changes were proposed in this pull request? The #15627 broke functionality with yarn --files --archives does not accept any files. This patch ensures that --files and --archives accept unique files. ## How was this patch tested? A. I added unit tests. B. Also, manually tested --files with --archives to throw exception if duplicate files are specified and continue if unique files are specified. Author: Kishor Patil <kpatil@yahoo-inc.com> Closes#15810 from kishorvpatil/SPARK18357. (cherry picked from commit 245e5a2) Signed-off-by: Tom Graves <tgraves@yahoo-inc.com>

## What changes were proposed in this pull request? The apache#15627 broke functionality with yarn --files --archives does not accept any files. This patch ensures that --files and --archives accept unique files. ## How was this patch tested? A. I added unit tests. B. Also, manually tested --files with --archives to throw exception if duplicate files are specified and continue if unique files are specified. Author: Kishor Patil <kpatil@yahoo-inc.com> Closesapache#15810 from kishorvpatil/SPARK18357.

ueshin · 2016-11-09T07:32:24Z

@kishorvpatil Thank you for fixing this!

… --files and --archives ## What changes were proposed in this pull request? During spark-submit, if yarn dist cache is instructed to add same file under --files and --archives, This code change ensures the spark yarn distributed cache behaviour is retained i.e. to warn and fail if same files is mentioned in both --files and --archives. ## How was this patch tested? Manually tested: 1. if same jar is mentioned in --jars and --files it will continue to submit the job. - basically functionality [SPARK-14423] apache#12203 is unchanged 1. if same file is mentioned in --files and --archives it will fail to submit the job. Please review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening a pull request. … under archives and files Author: Kishor Patil <kpatil@yahoo-inc.com> Closesapache#15627 from kishorvpatil/spark18099.

## What changes were proposed in this pull request? The apache#15627 broke functionality with yarn --files --archives does not accept any files. This patch ensures that --files and --archives accept unique files. ## How was this patch tested? A. I added unit tests. B. Also, manually tested --files with --archives to throw exception if duplicate files are specified and continue if unique files are specified. Author: Kishor Patil <kpatil@yahoo-inc.com> Closesapache#15810 from kishorvpatil/SPARK18357.

kishorvpatil force-pushed the spark18099 branch from 9bb1623 to a3eb6d4Compare October 26, 2016 20:41

Dist cache yarn during submit should throw error for adding same file…
2c55fc2
… under archives and files

kishorvpatil force-pushed the spark18099 branch from a3eb6d4 to 2c55fc2Compare October 27, 2016 15:15

tgravescs reviewed Oct 27, 2016
View reviewed changes

Convert required to IllegalArgumentException
a1dc858

tgravescs reviewed Oct 28, 2016
View reviewed changes

Reword exception message
33f95ab

tgravescs reviewed Oct 28, 2016
View reviewed changes

Adding comment and unit tests
f797481

tgravescs reviewed Nov 1, 2016
View reviewed changes

Updating unit test cover all three scenarios.
51eefa5

asfgit closed this in 098e4caNov 3, 2016

ueshin reviewed Nov 8, 2016
View reviewed changes

kishorvpatil mentioned this pull request Nov 8, 2016
[SPARK-18357] Fix yarn files/archive broken issue andd unit tests #15810
Closed

caneGuy mentioned this pull request Aug 29, 2017
[SPARK-21855][YARN] Should print error when upload same file multiple tim… #19073
Closed

Uh oh!

Conversation

kishorvpatil commented Oct 25, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Oct 25, 2016

Uh oh!

tgravescs commented Oct 26, 2016

Uh oh!

SparkQA commented Oct 26, 2016

Uh oh!

tgravescs commented Oct 27, 2016

Uh oh!

tgravescs commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 27, 2016

Uh oh!

SparkQA commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 28, 2016

Uh oh!

SparkQA commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 3, 2016

Uh oh!

tgravescs commented Nov 3, 2016

Uh oh!

ueshin commented Nov 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Nov 8, 2016

Uh oh!

tgravescs commented Nov 8, 2016

Uh oh!

kishorvpatil commented Nov 8, 2016

Uh oh!

ueshin commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants