Skip to content

[SPARK-14705][YARN]support Multiple FileSystem for YARN STAGING DIR#12473

Closed
lianhuiwang wants to merge 5 commits intoapache:masterfrom
lianhuiwang:SPARK-14705
Closed

[SPARK-14705][YARN]support Multiple FileSystem for YARN STAGING DIR#12473
lianhuiwang wants to merge 5 commits intoapache:masterfrom
lianhuiwang:SPARK-14705

Conversation

@lianhuiwang
Copy link
Contributor

@lianhuiwang lianhuiwang commented Apr 18, 2016

What changes were proposed in this pull request?

In SPARK-13063, It makes the SPARK YARN STAGING DIR as configurable. But it only support default FileSystem. If there are many clusters, It can be different FileSystem for different cluster in our spark.

How was this patch tested?

I have tested it successfully with following commands:
MASTER=yarn-client ./bin/spark-shell --conf spark.yarn.stagingDir=hdfs:namenode2/temp
$SPARK_HOME/bin/spark-submit --conf spark.yarn.stagingDir=hdfs:namenode2/temp

// and add them as local resources to the application master.
val fs = FileSystem.get(hadoopConf)
val dst = getAppStagingDirPath(sparkConf, fs, appStagingDir)
val dst = new Path(appStagingBaseDir, appStagingDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could pass appStagingDir as a Path to this method and save some duplication; same for setupLaunchEnv below.

This whole class could use some cleanup in that regard, but these two are pretty low-hanging fruit.

@SparkQA
Copy link

SparkQA commented Apr 18, 2016

Test build #56064 has finished for PR 12473 at commit 0ba4de8.

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

@tgravescs
Copy link
Contributor

tgravescs commented Apr 18, 2016

test says NA, what testing have you done with this?

In the very least I would like to see manual regression test hdfs and I assume you are making this to talk to some other filesystem, so what other filesystem was it tested with?

@lianhuiwang
Copy link
Contributor Author

lianhuiwang commented Apr 19, 2016

@vanzin yes, I update code with your comments.Thanks.
@tgravescs I have tested it on my spark using spark-shell and spark-submit, I have updated it.Thanks.

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56182 has finished for PR 12473 at commit c0374af.

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

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56186 has finished for PR 12473 at commit 59eb03c.

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

@jerryshao
Copy link
Contributor

So my understanding is that actually supporting different HDFS other than default one, not multiple HDFS, is that right?

@lianhuiwang
Copy link
Contributor Author

@jerryshao Yes, what you said is right.

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56188 has finished for PR 12473 at commit 61b51f2.

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

@HyukjinKwon
Copy link
Member

(This is super minor but I remember I was told it might be better if those cc are added in comments not in the description because PR description is the place where to describe the PR.)

@vanzin
Copy link
Contributor

vanzin commented Apr 20, 2016

@HyukjinKwon the merge scripts clean up "@" references from the PR summary.

@vanzin
Copy link
Contributor

vanzin commented Apr 20, 2016

LGTM, merging to master.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Apr 20, 2016

@vanzin Thank you! but it might still look a bit weird that there is cc in PR description above maybe.

@asfgit asfgit closed this in 4514aeb Apr 20, 2016
@lianhuiwang
Copy link
Contributor Author

@HyukjinKwon @vanzin Thanks. I have updated PR description. But @vanzin have merged to master before. So I think it does not matter for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants