-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-4826] Fix generation of temp file names in WAL tests #3704
Conversation
Test build #24464 has started for PR 3704 at commit
|
val writer = new WriteAheadLogWriter(new File(dir, Random.nextString(10)).toString, hadoopConf) | ||
val logFilePath = { | ||
val f = File.createTempFile("wal", null, dir) | ||
assert(f.delete()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might look race prone (deleting a file and hoping that someone else won't come along and write it in the meantime), but it should be safe because:
- Different Jenkins builds will have different temp directories (
dir
). - Within a JVM, multiple calls to
createTempFile
will never return the same pathname (see Javadoc).
This looks good to me, though the approach does not make it obvious why this approach was chosen (of course you can figure out in this context, but imagine reading this code a year later). I think the other two are slightly simpler approaches by ensuring unique names on file creation. |
I buy your explanation, given the javadoc, so this LGTM. But I think a cleaner approach that doesn't require reasoning like that to convince people would be to just use a different temp dir per test (i.e. use |
The root issue is that this code is trying to return a unique file system path that meets two conditions:
I think this was the intent expressed by the original |
I assume that frameworks handle that automatically (e.g. by having multiple instances of the test class), otherwise you could never parallelize tests that use "before" initializers. I'm pretty sure that works as intented at least with JUnit, but not super familiar with scalatest internals. |
Alright, I've simplified things to move the temp. dir creation to |
+1. The latest changes look good. |
} | ||
|
||
override def afterEach(): Unit = { | ||
dir.delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work if the dir is not empty. You could use Utils.createTempDir()
and, optionally, Utils.deleteRecursively()
(since createTempDir
already takes care of that for you).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; I've fixed this.
Test build #24466 has started for PR 3704 at commit
|
Test build #24467 has started for PR 3704 at commit
|
Test build #24464 has finished for PR 3704 at commit
|
Test PASSed. |
Test build #24466 has finished for PR 3704 at commit
|
Test PASSed. |
Test build #24467 has finished for PR 3704 at commit
|
Test PASSed. |
Looks like a robust approach to me. |
Alright, I'm going to merge this. Thanks for looking this over! |
This PR should fix SPARK-4826, an issue where a bug in how we generate temp. file names was causing spurious test failures in the write ahead log suites. Closes #3695. Closes #3701. Author: Josh Rosen <joshrosen@databricks.com> Closes #3704 from JoshRosen/SPARK-4826 and squashes the following commits: f2307f5 [Josh Rosen] Use Spark Utils class for directory creation/deletion a693ddb [Josh Rosen] remove unused Random import b275e41 [Josh Rosen] Move creation of temp. dir to beforeEach/afterEach. 9362919 [Josh Rosen] [SPARK-4826] Fix bug in generation of temp file names. in WAL suites. 86c1944 [Josh Rosen] Revert "HOTFIX: Disabling failing block manager test" (cherry picked from commit f6b8591) Signed-off-by: Josh Rosen <joshrosen@databricks.com>
This PR should fix SPARK-4826, an issue where a bug in how we generate temp. file names was causing spurious test failures in the write ahead log suites.
Closes #3695.
Closes #3701.