[BEAM-57] Makes FileBasedSink use a temporary directory by jkff · Pull Request #1050 · apache/beam

jkff · 2016-10-04T22:51:20Z

When writing to /path/to/foo, temporary files would be written to /path/to/foo-temp-$uid (or something like that), i.e. as siblings of the final output. That could lead to issues like http://stackoverflow.com/q/39822859/278042

Now, temporary files are written to a path like: /path/to/temp-beam-foo-$date/$uid. This way, the temporary files won't match the same glob as the final output files (even though they may still fail to be deleted due to eventual consistency issues).

jkff · 2016-10-04T22:59:18Z

R: @dhalperi

dhalperi · 2016-10-14T05:59:17Z

R: @lukecwik

dhalperi

My 2c: this seems an overly invasive change for what it's trying to do. I would just write the temp folders to a temp directory inside of the path.

Other comments:

Don't use Path.resolve-like functions -- may not work on all filesystems, which is why we have IOChannelFactory.resolve
Sibling resolution seems wrong and/or not guaranteed to work. Subdirectory seems simpler and more reliable.

Would like Luke's eyes to get some of the finer details, (e.g., does all this work on Windows?) and given that I've continued to be too randomized for this in the last 9 days Eugene could use the sharding.

dhalperi · 2016-10-14T05:56:41Z

examples/java8/src/test/java/org/apache/beam/examples/MinimalWordCountJava8Test.java

     .apply(MapElements
         .via((KV<String, Long> wordCount) -> wordCount.getKey() + ": " + wordCount.getValue())
         .withOutputType(TypeDescriptors.strings()))
-     .apply(TextIO.Write.to("gs://YOUR_OUTPUT_BUCKET/AND_OUTPUT_PREFIX"));


revert unrelated change?

lukecwik · 2016-10-14T21:45:48Z

Both subdirectories and siblings have issues but subdirectory seems to be the lesser of two evils as the fallback. There is also PipelineOptions.tempLocation but that may not work at all since the IOChannelFactory may not be equivalent or there will be other underlying storage differences like the GCS bucket may be part of a different GCP project.

In the referenced example of the person reading the pipeline in later, paths that are ** will still encompass the temp files. Sibling fails when the output location is a GCS bucket since you can't just create arbitrary siblings at that level or in a local file system the path is "/".

Does it make sense for the IOChannelFactory to own creating temp file locations?

jkff · 2016-10-14T21:56:15Z

@dhalperi:

If we write to a temp directory inside the path, a user's glob like "*/" will match the temp files - that's why I opted for a sibling, it seems cleaner in terms of isolation.
I used Path.resolve on Path objects provided by the IOChannelFactory (Path is an interface), so it should be ok. And I, indeed, had to add sibling resolution methods to our IOChannelFactory's to make it work.

@lukecwik:

Agreed that we can't use tempLocation for the reasons you listed.
Agreed that this solution won't work when the user wants to write to a root path (raw GCS bucket or /something on a filesystem). Perhaps in this case we should require them to provide a temp location, since there's no obvious safe choice the SDK can make for them?
By "creating temp file locations", you mean "I want to write to this path, give me a scratchpad for that" - i.e. move the generation of a temp location from FileBasedSink to IOChannelFactory?

jkff · 2016-10-19T18:26:30Z

Ping.

lukecwik · 2016-10-19T22:31:23Z

I think this is something we should bring back to the dev list since there isn't a clean solution either way and both have drawbacks.

jkff · 2016-10-27T18:25:54Z

Discussion happening on https://www.mail-archive.com/dev@beam.incubator.apache.org/msg01726.html

dhalperi · 2016-11-01T20:44:07Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java

+                "temp-beam-"
+                    + baseOutputPath.getFileName()
+                    + "-"
+                    + Instant.now().toString(DateTimeFormat.forPattern("yyyy-MM-DD_HH-mm-ss")))


This is highlighting a general confusion (on my part) between Path and URI. It seems like we make this mistake all over the place, so this need not block this PR.

cc: @peihe

Original thought, can be ignored: maybe this should be toURI().toString() ?

When writing to /path/to/foo, temporary files would be written to /path/too/foo-temp-$uid (or something like that), i.e. as siblings of the final output. That could lead to issues like http://stackoverflow.com/q/39822859/278042 Now, temporary files are written to a path like: /path/too/temp-beam-foo-$date/$uid.

jkff · 2016-11-01T21:58:43Z

Rebased; per thread on beam-dev@ and per offline discussion with @dhalperi the approach the PR is taking is okay.

jkff · 2016-11-02T00:20:41Z

Failures look flaky (connection errors with apache repos).

dhalperi · 2016-11-02T00:22:48Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/gcsfs/GcsPath.java

-    throw new UnsupportedOperationException();
+    GcsPath parent = getParent();
+    return (parent == null) ? fromUri(other) : parent.resolve(other);
  }


unit tests of 3 new implementations?

dhalperi · 2016-11-03T19:47:49Z

LGTM, merged.

apache/beam#1050 apache/beam#1278

This is a weaker but backward-compatible version of respective Beam changes: apache/beam#1050 apache/beam#1278

dhalperi requested changes Oct 14, 2016

View reviewed changes

dhalperi reviewed Nov 1, 2016

View reviewed changes

jkff changed the title ~~Makes FileBasedSink use a temporary directory~~ [BEAM-57] Makes FileBasedSink use a temporary directory Nov 1, 2016

jkff force-pushed the file-sink-tmp-dir branch from 1c34acd to fa3b125 Compare November 1, 2016 21:58

dhalperi reviewed Nov 2, 2016

View reviewed changes

Added tests for 2 new GcsPath methods and removed the 3rd

e086d76

asfgit closed this in 55d9519 Nov 3, 2016

jkff deleted the file-sink-tmp-dir branch November 3, 2016 21:10

mxm mentioned this pull request Nov 4, 2016

[BEAM-896] adjust ReadSourceITCase to exclude Beam temporary files #1283

Closed

jkff added a commit to jkff/DataflowJavaSDK that referenced this pull request Nov 10, 2016

Backporting FileBasedSink changes from Beam

e469a22

apache/beam#1050 apache/beam#1278

jkff mentioned this pull request Nov 10, 2016

Backporting FileBasedSink changes from Beam GoogleCloudPlatform/DataflowJavaSDK#482

Closed

jkff added a commit to jkff/DataflowJavaSDK that referenced this pull request Nov 14, 2016

FileBasedSink removes temp files from successful bundles

fb4f8e3

This is a weaker but backward-compatible version of respective Beam changes: apache/beam#1050 apache/beam#1278

jkff added a commit to jkff/DataflowJavaSDK that referenced this pull request Nov 14, 2016

FileBasedSink removes temp files from successful bundles

fdbb51f

This is a weaker but backward-compatible version of respective Beam changes: apache/beam#1050 apache/beam#1278

Conversation

jkff commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkff commented Oct 4, 2016

Uh oh!

dhalperi commented Oct 14, 2016

Uh oh!

dhalperi left a comment

Choose a reason for hiding this comment

Uh oh!

dhalperi Oct 14, 2016

Choose a reason for hiding this comment

Uh oh!

dhalperi Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Oct 14, 2016

Uh oh!

jkff commented Oct 14, 2016

Uh oh!

jkff commented Oct 19, 2016

Uh oh!

lukecwik commented Oct 19, 2016

Uh oh!

jkff commented Oct 27, 2016

Uh oh!

dhalperi Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

jkff commented Nov 1, 2016

Uh oh!

jkff commented Nov 2, 2016

Uh oh!

dhalperi Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

jkff Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

dhalperi commented Nov 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkff commented Oct 4, 2016 •

edited

Loading