Skip to content

[SPARK-4826][Streaming] - Create unique file names for each test in WALS... #3695

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.

Sign up for GitHub

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

Jump to bottom
Closed
harishreedharan wants to merge 4 commits into from
Closed

[SPARK-4826][Streaming] - Create unique file names for each test in WALS... #3695

harishreedharan wants to merge 4 commits into from

Conversation

@harishreedharan
Copy link
Contributor

@harishreedharan harishreedharan commented Dec 15, 2014

...uite to avoid conflicts.

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24446 has started for PR 3695 at commit ae63f35.

  • This patch merges cleanly.

@harishreedharan
Copy link
Contributor Author

harishreedharan commented Dec 15, 2014

@tdas @JoshRosen Since the directories are created atomically each time the suite is run, conflicts reported in SPARK-4826 was likely caused by multiple tests running at the same time and using the same filename (since the writeLogSegments method is not thread-safe).

@harishreedharan
Use FileUtils.deleteDirectory(..) instead of File.delete, since the l…
2948408 
…atter cannot delete directories that are not empty.
@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24447 has started for PR 3695 at commit 2948408.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24447 has finished for PR 3695 at commit 2948408.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 15, 2014

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

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24448 has started for PR 3695 at commit 2e2bf28.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24446 has finished for PR 3695 at commit ae63f35.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 15, 2014

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

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24448 has finished for PR 3695 at commit 2e2bf28.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 15, 2014

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

srowen
srowen reviewed Dec 15, 2014
View reviewed changes
streaming/src/test/scala/org/apache/spark/streaming/rdd/WriteAheadLogBackedBlockRDDSuite.scala Outdated
Copy link
Member

@srowen srowen Dec 15, 2014

Choose a reason for hiding this comment

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

Let's not bring in commons-io again for this. There is already a method in Utils that does this.

@srowen
Copy link
Member

srowen commented Dec 15, 2014

The fix in #3701 looks simpler but it's fixing a different supposed cause. Is it really that the random method produces the same string many times, because of a race condition or something? seems like synchronization would fix that then. The alternate method in the other PR may do the same.

@harishreedharan
Copy link
Contributor Author

harishreedharan commented Dec 15, 2014

I can't be sure which one is causing the issue but this one should take care of both. Since thee file names are being updated atomically - each test will get a unique file without random causing issues again.

@JoshRosen
Copy link
Contributor

JoshRosen commented Dec 15, 2014

[...] conflicts reported in SPARK-4826 was likely caused by multiple tests running at the same time and using the same filename.

Here's what I find confusing, though: the Jenkins master builds (and branch-1.2 builds) don't have any parallelism within a single build: only one thread of control should be executing in a given instance of WriteAheadLogBackedBlockRDDSuite. There might be multiple instances of WriteAheadLogBackedBlockRDDSuite executing in different Jenkins builds, but I would expect that the different Jenkins builds would have different values of Files.createTempDir() and hence would write to different output locations.

Are you saying that within the same Jenkins build / instance of WriteAheadLogBackedBlockRDDSuite, we are having two separate tests that are choosing to write to the same subdirectory / file inside of Files.createTempDir()?

Maybe something is implicitly calling Random.setSeed() between test cases, causing the tests to pick the same subdirectory. If that's the case, we might be able to fix things by moving the directory deletion call to a afterEach() method or by using Files.createTempFile within that temp directory in order to pick the filename.

@harishreedharan
Copy link
Contributor Author

harishreedharan commented Dec 15, 2014

So, it could also what #3701 mentions, where the random class is returning the same string for each call - in which case every tests following the first one fails. This PR should fix that too, since each test will get a unique file name. Basically, I am trying to fix boh the cases -
(1) Random returns the same string
(2) tests executing in parallel.

@JoshRosen JoshRosen mentioned this pull request Dec 15, 2014
Closed
@JoshRosen
Copy link
Contributor

JoshRosen commented Dec 15, 2014

I've opened a new PR with what I think is is a more direct approach to fixing this issue: #3704. Please take a look and let me know what you think.

@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24465 has started for PR 3695 at commit 7d0044b.

  • This patch merges cleanly.

@harishreedharan
Copy link
Contributor Author

harishreedharan commented Dec 15, 2014

Closing this as #3704 takes care of this issue.

@harishreedharan harishreedharan deleted the WALSuite-Fix branch December 15, 2014 19:54
@SparkQA
Copy link

SparkQA commented Dec 15, 2014

Test build #24465 has finished for PR 3695 at commit 7d0044b.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 15, 2014

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

asfgit pushed a commit that referenced this pull request Dec 15, 2014
@JoshRosen
[SPARK-4826] Fix generation of temp file names in WAL tests
c5a9ae6 
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>
asfgit pushed a commit that referenced this pull request Dec 15, 2014
@JoshRosen
[SPARK-4826] Fix generation of temp file names in WAL tests
f6b8591 
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"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@harishreedharan @SparkQA @AmplabJenkins @srowen @JoshRosen