[SPARK-4764] Ensure that files are fetched atomically #2855

preaudc · 2014-10-20T10:14:30Z

tempFile is created in the same directory than targetFile, so that the
move from tempFile to targetFile is always atomic

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic

AmplabJenkins · 2014-10-20T10:17:10Z

Can one of the admins verify this patch?

AmplabJenkins · 2014-10-21T23:13:49Z

Can one of the admins verify this patch?

andrewor14 · 2014-10-28T20:16:37Z

add to whitelist

SparkQA · 2014-10-28T20:19:59Z

QA tests have started for PR 2855 at commit 8ea871f.

This patch does not merge cleanly.

SparkQA · 2014-10-28T21:52:22Z

QA tests have finished for PR 2855 at commit 8ea871f.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-10-28T21:52:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22374/
Test PASSed.

SparkQA · 2014-11-03T13:29:47Z

Test build #22815 has started for PR 2855 at commit 54419ae.

This patch merges cleanly.

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic

SparkQA · 2014-11-03T13:44:54Z

Test build #22816 has started for PR 2855 at commit 9ba89ca.

This patch merges cleanly.

preaudc · 2014-11-03T13:51:45Z

I've just resync my local fork so that the patch merges cleanly (i.e. there are no other modifications compared to the initial patch)

SparkQA · 2014-11-03T15:00:51Z

Test build #22815 has finished for PR 2855 at commit 54419ae.

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

AmplabJenkins · 2014-11-03T15:00:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22815/
Test PASSed.

SparkQA · 2014-11-03T15:09:39Z

Test build #22816 has finished for PR 2855 at commit 9ba89ca.

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

AmplabJenkins · 2014-11-03T15:09:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22816/
Test PASSed.

andrewor14 · 2014-11-06T23:45:19Z

Hey @preaudc can you explain how this fixes SPARK-3967?

andrewor14 · 2014-11-06T23:48:15Z

Also, how does this relate to #2848? Can you and @ryan-williams sort out which PR makes more sense?

ryan-williams · 2014-11-07T00:28:41Z

#2484 seems beneficial no matter what; why would we ever copy a JAR we've just confirmed is identical to the thing already at the destination?

ryan-williams · 2014-11-07T00:31:19Z

I don't think these two PRs are necessarily that related though, other than each having been borne as a workaround to the same stack-trace. There may be a common root cause but I don't know, so no need to consider them as coupled or mutually exclusive.

preaudc · 2014-11-07T09:39:51Z

As @ryan-williams pointed out, this is initially only a workaround to SPARK-3967.
I have still no idea why the move fails (with a "Permission denied") when the source and target files are not on the same partition (it is no more atomic, but it should succeed anyway).
I have made this patch because it does not seem necessary to download the file into another local directory then move it (it may cause a copy instead of a rename, and does in fact here).

SparkQA · 2014-11-19T19:30:50Z

Test build #23617 has started for PR 2855 at commit 9ba89ca.

This patch merges cleanly.

SparkQA · 2014-11-19T20:57:55Z

Test build #23617 has finished for PR 2855 at commit 9ba89ca.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-11-19T20:57:58Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23617/
Test FAILed.

preaudc · 2014-11-21T13:29:27Z

I have no clue what could bring this MiMa test failure, and how to fix it. Can anybody give me a hand?

JoshRosen · 2014-12-01T03:08:28Z

Jenkins, retest this please.

SparkQA · 2014-12-01T03:15:37Z

Test build #23968 has started for PR 2855 at commit 9ba89ca.

This patch merges cleanly.

SparkQA · 2014-12-01T04:33:50Z

Test build #23968 has finished for PR 2855 at commit 9ba89ca.

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

AmplabJenkins · 2014-12-01T04:33:54Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23968/
Test PASSed.

JoshRosen · 2014-12-05T01:33:25Z

This looks good to me, since I think that there's no reason to download the temporary file to a different directory than its intended destination. I'd like to be able to merge this independently of #2848, so do you mind creating a new JIRA that describes only this change rather than SPARK-3967 and updating this PR's title to reference that JIRA?

preaudc · 2014-12-08T15:29:21Z

Thanks for the review, @JoshRosen, I've created a new JIRA as requested.

JoshRosen · 2014-12-08T19:43:45Z

Thanks for creating the new JIRA. This looks good to me, so I'm going to merge it into master and branch-1.1 for now (I've added a backport-needed label to the JIRA so that we remember to merge this into branch-1.2 after the 1.2.0 vote ends). Thanks!

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic Author: Christophe Préaud <christophe.preaud@kelkoo.com> Closes #2855 from preaudc/master and squashes the following commits: 9ba89ca [Christophe Préaud] Ensure that files are fetched atomically 54419ae [Christophe Préaud] Merge remote-tracking branch 'upstream/master' c6a5590 [Christophe Préaud] Revert commit 8ea871f 7456a33 [Christophe Préaud] Merge remote-tracking branch 'upstream/master' 8ea871f [Christophe Préaud] Ensure that files are fetched atomically (cherry picked from commit ab2abcb) Signed-off-by: Josh Rosen <rosenville@gmail.com> Conflicts: core/src/main/scala/org/apache/spark/util/Utils.scala

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic Author: Christophe Préaud <christophe.preaud@kelkoo.com> Closes #2855 from preaudc/master and squashes the following commits: 9ba89ca [Christophe Préaud] Ensure that files are fetched atomically 54419ae [Christophe Préaud] Merge remote-tracking branch 'upstream/master' c6a5590 [Christophe Préaud] Revert commit 8ea871f 7456a33 [Christophe Préaud] Merge remote-tracking branch 'upstream/master' 8ea871f [Christophe Préaud] Ensure that files are fetched atomically (cherry picked from commit ab2abcb) Signed-off-by: Josh Rosen <joshrosen@databricks.com>

JoshRosen · 2014-12-17T20:10:18Z

I've finished backporting this to branch-1.2.

zsxwing · 2014-12-31T01:57:41Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -425,8 +425,7 @@ private[spark] object Utils extends Logging {
      conf: SparkConf,
      securityMgr: SecurityManager,
      hadoopConf: Configuration) {
-    val tempDir = getLocalDir(conf)
-    val tempFile =  File.createTempFile("fetchFileTemp", null, new File(tempDir))
+    val tempFile = File.createTempFile("fetchFileTemp", null, new File(targetDir.getAbsolutePath))


Why need to create a new File here: new File(targetDir.getAbsolutePath)? Can't we use targetDir directly?

Yeah, we probably can just use targetDir directly; I guess I overlooked this when reviewing the patch.

preaudc · 2015-01-05T12:37:26Z

Yes, my bad, targetDir is indeed already a File. @JoshRosen , how could I fix this, should I create a new pull request, or can this one be reopened?

JoshRosen · 2015-01-05T19:52:13Z

@preaudc We can't re-open this PR, so you'd have to create a new one. This is a really minor issue, though, so I'm not in a huge rush to fix it (it's more of a code-cleanliness issue than a correctness issue).

Ensure that files are fetched atomically

8ea871f

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic

Merge remote-tracking branch 'upstream/master'

7456a33

Christophe Préaud added 2 commits November 3, 2014 14:25

Revert commit 8ea871f

c6a5590

Merge remote-tracking branch 'upstream/master'

54419ae

Ensure that files are fetched atomically

9ba89ca

tempFile is created in the same directory than targetFile, so that the move from tempFile to targetFile is always atomic

preaudc changed the title ~~[SPARK-3967] Ensure that files are fetched atomically~~ [SPARK-4764] Ensure that files are fetched atomically Dec 5, 2014

asfgit closed this in ab2abcb Dec 8, 2014

zsxwing reviewed Dec 31, 2014
View reviewed changes

[SPARK-4764] Ensure that files are fetched atomically #2855

[SPARK-4764] Ensure that files are fetched atomically #2855

Uh oh!

Conversation

preaudc commented Oct 20, 2014

Uh oh!

AmplabJenkins commented Oct 20, 2014

Uh oh!

AmplabJenkins commented Oct 21, 2014

Uh oh!

andrewor14 commented Oct 28, 2014

Uh oh!

SparkQA commented Oct 28, 2014

Uh oh!

SparkQA commented Oct 28, 2014

Uh oh!

AmplabJenkins commented Oct 28, 2014

Uh oh!

SparkQA commented Nov 3, 2014

Uh oh!

SparkQA commented Nov 3, 2014

Uh oh!

preaudc commented Nov 3, 2014

Uh oh!

SparkQA commented Nov 3, 2014

Uh oh!

AmplabJenkins commented Nov 3, 2014

Uh oh!

SparkQA commented Nov 3, 2014

Uh oh!

AmplabJenkins commented Nov 3, 2014

Uh oh!

andrewor14 commented Nov 6, 2014

Uh oh!

andrewor14 commented Nov 6, 2014

Uh oh!

ryan-williams commented Nov 7, 2014

Uh oh!

ryan-williams commented Nov 7, 2014

Uh oh!

preaudc commented Nov 7, 2014

Uh oh!

SparkQA commented Nov 19, 2014

Uh oh!

SparkQA commented Nov 19, 2014

Uh oh!

AmplabJenkins commented Nov 19, 2014

Uh oh!

preaudc commented Nov 21, 2014

Uh oh!

JoshRosen commented Dec 1, 2014

Uh oh!

SparkQA commented Dec 1, 2014

Uh oh!

SparkQA commented Dec 1, 2014

Uh oh!

AmplabJenkins commented Dec 1, 2014

Uh oh!

JoshRosen commented Dec 5, 2014

Uh oh!

preaudc commented Dec 8, 2014

Uh oh!

JoshRosen commented Dec 8, 2014

Uh oh!

JoshRosen commented Dec 17, 2014

Uh oh!

zsxwing Dec 31, 2014

Choose a reason for hiding this comment

Uh oh!

JoshRosen Jan 4, 2015

Choose a reason for hiding this comment

Uh oh!

preaudc commented Jan 5, 2015

Uh oh!

JoshRosen commented Jan 5, 2015

Uh oh!

Uh oh!