You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StephanEwen <gi...@git.apache.org> on 2016/12/13 20:05:50 UTC
[GitHub] flink pull request #2999: [FLINK-5332] [core] Synchronize FileSystem::initOu...
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/2999
[FLINK-5332] [core] Synchronize FileSystem::initOutPathLocalFS() to prevent lost files/directories when called concurrently
This is mainly relevant to tests and Local Mini Cluster executions.
The `FileOutputFormat` and its subclasses rely on `FileSystem::initOutPathLocalFS()` to prepare the output directory. When multiple parallel output writers call that method, there is a slim chance that one parallel threads deletes the others directory. The checks that the method has are not bullet proof.
I believe that this is the cause for many Travis test instabilities that we observed over time.
Simply synchronizing that method per process should do the trick. Since it is a rare initialization method, and only relevant in tests & local mini cluster executions, it should be a price that is okay to pay. I see no other way, as we do not have simple access to an atomic "check and delete and recreate" file operation.
The synchronization also makes many "re-try" code paths obsolete (there should be no re-tries needed on proper file systems).
### Tests
This is tricky to test. The test in `InitOutputPathTest.java` uses a series of latch to re-produce the problematic thread execution interleaving to validate the problem. The properly fixed variant cannot use that interleaving (because it fixes the problem, duh), but pushes the thread interleaving best-effort towards the case where the problem would occur, were the method not properly synchronized. Sounds weird, I know.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/StephanEwen/incubator-flink fs_fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2999.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2999
----
commit 7a4d5b2f53e3b8c27a5224e405cf1b671f474a58
Author: Stephan Ewen <se...@apache.org>
Date: 2016-12-13T18:15:11Z
[tests] Add 'CheckedThread' as a common test utility
commit e25b436f1de18be0dc2c3b02b82bf0b8203f0b44
Author: Stephan Ewen <se...@apache.org>
Date: 2016-12-13T18:12:12Z
[FLINK-5332] [core] Synchronize FileSystem::initOutPathLocalFS() to prevent lost files when called concurrently.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2999: [FLINK-5332] [core] Synchronize FileSystem::initOutPathLo...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/2999
Changes look good to me. Great fix you've implemented there @StephanEwen. This will hopefully make some of the failing travis test cases stable again :-) +1 for merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2999: [FLINK-5332] [core] Synchronize FileSystem::initOutPathLo...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2999
Thanks, will merge this...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2999: [FLINK-5332] [core] Synchronize FileSystem::initOutPathLo...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2999
I think this should go into 1.2 - it is quite a bug for local testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2999: [FLINK-5332] [core] Synchronize FileSystem::initOu...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/2999
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2999: [FLINK-5332] [core] Synchronize FileSystem::initOutPathLo...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2999
Manually merged in 2f3ad58b7b73463aa1827baef0eb2e9d87fdb882
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---