[SPARK-19677][SS] Committing a delta file atop an existing one should not fail on HDFS #17012

vitillo · 2017-02-21T08:36:58Z

What changes were proposed in this pull request?

HDFSBackedStateStoreProvider fails to rename files on HDFS but not on the local filesystem. According to the implementation notes of rename(), the behavior of the local filesystem and HDFS varies:

Destination exists and is a file
Renaming a file atop an existing file is specified as failing, raising an exception.

Local FileSystem : the rename succeeds; the destination file is replaced by the source file.

HDFS : The rename fails, no exception is raised. Instead the method call simply returns false.

This patch ensures that rename() isn't called if the destination file already exists. It's still semantically correct because Structured Streaming requires that rerunning a batch should generate the same output.

How was this patch tested?

This patch was tested by running StateStoreSuite.

vitillo · 2017-02-21T08:40:53Z

@tdas @zsxwing

zsxwing · 2017-02-22T07:03:40Z

The fix looks good to me. But the test doesn't test anything. Right?

vitillo · 2017-02-22T08:33:16Z

@zsxwing The test is there just to make sure the new code path is executed during tests. It probably makes more sense to create a fake filesystem type that adheres to the rename semantic of HDFS to test this.

vitillo · 2017-02-23T09:31:27Z

@zsxwing I have rewritten the test code.

hejix · 2017-02-24T23:04:51Z

@zsxwing Is it possible for you to give some approximate timeframe for when this might be accepted? A week or month? Since we are blocked in our spark streaming adoption (this HDFS error happens not only in restart but also in first run of our more complex aggregating drivers), I don't know if I should wait for this fix to make it into the nightly build soon or build a custom dist ourselves and update our automation. Any input as to the fix timeline would be greatly appreciated!

Also this fix handles the same issue as SPARK-19645 which has a different solution. Is this simple solution sufficient or the more complex fix in #16980 required?

zsxwing · 2017-02-25T00:12:08Z

...main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala

Actually, I think we should not delete the file. Otherwise, it will break speculation. E.g., there is also a speculation ask running. It wrote successfully and just reported to the driver that the task was successful. At the same time, another task just deleted the final file wrongly because the file exists.

The correct way is just skipping rename. We can depend on the basic assumption of Structured Streaming that the same batch should contain the same data.

hejix · 2017-02-28T00:15:25Z

Just some feedback that I did some initial regression testing with this pull request on a full YARN (v2.7.3) 4 node cluster on GCP and it appears to have fixed the two issues we had- our structured streaming drivers now restart normally and our complex aggregation driver runs for the first time.

mridulm · 2017-02-28T01:18:18Z

What is the proposed semantics from this PR now ?

If file exists, ignore.
If file does not exist, try to rename - if fails, throw exception.

Is this right ? If yes, the PR title/description should be changed.

zsxwing · 2017-02-28T01:39:51Z

ok to test

zsxwing · 2017-02-28T01:39:55Z

Is this right ? If yes, the PR title/description should be changed.

+1

SparkQA · 2017-02-28T01:44:25Z

Test build #73548 has finished for PR 17012 at commit 530c027.

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

zsxwing · 2017-02-28T01:54:02Z

...main/scala/org/apache/spark/sql/execution/streaming/state/HDFSBackedStateStoreProvider.scala

Could you add our discussion to the comment? Such as

// scalastyle:off // Renaming a file atop an existing one fails on HDFS // (http://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/filesystem.html). // Hence we should either skip the rename step or delete the target file. Because deleting the // target file will beak speculation, skipping the rename step is the only choice. It's still // semantically correct because Structured Streaming requires rerunning a batch should // generate the same output. (SPARK-19677) // scalastyle:on

zsxwing · 2017-02-28T01:55:19Z

sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreSuite.scala

nit: SPARK-19677: Renaming a file atop an existing one on HDFS

zsxwing · 2017-02-28T01:55:51Z

Just two minor comments. Otherwise, LGTM.

zsxwing · 2017-02-28T02:07:47Z

sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreSuite.scala

nit: indent

SparkQA · 2017-02-28T15:30:02Z

Test build #73589 has finished for PR 17012 at commit de228a4.

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

zsxwing · 2017-02-28T18:47:56Z

LGTM. Thanks! Merging to master, 2.1 and 2.0.

… not fail on HDFS ## What changes were proposed in this pull request? HDFSBackedStateStoreProvider fails to rename files on HDFS but not on the local filesystem. According to the [implementation notes](https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/filesystem.html) of `rename()`, the behavior of the local filesystem and HDFS varies: > Destination exists and is a file > Renaming a file atop an existing file is specified as failing, raising an exception. > - Local FileSystem : the rename succeeds; the destination file is replaced by the source file. > - HDFS : The rename fails, no exception is raised. Instead the method call simply returns false. This patch ensures that `rename()` isn't called if the destination file already exists. It's still semantically correct because Structured Streaming requires that rerunning a batch should generate the same output. ## How was this patch tested? This patch was tested by running `StateStoreSuite`. Author: Roberto Agostino Vitillo <[email protected]> Closes #17012 from vitillo/fix_rename.

… not fail on HDFS ## What changes were proposed in this pull request? HDFSBackedStateStoreProvider fails to rename files on HDFS but not on the local filesystem. According to the [implementation notes](https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/filesystem.html) of `rename()`, the behavior of the local filesystem and HDFS varies: > Destination exists and is a file > Renaming a file atop an existing file is specified as failing, raising an exception. > - Local FileSystem : the rename succeeds; the destination file is replaced by the source file. > - HDFS : The rename fails, no exception is raised. Instead the method call simply returns false. This patch ensures that `rename()` isn't called if the destination file already exists. It's still semantically correct because Structured Streaming requires that rerunning a batch should generate the same output. ## How was this patch tested? This patch was tested by running `StateStoreSuite`. Author: Roberto Agostino Vitillo <[email protected]> Closes #17012 from vitillo/fix_rename. (cherry picked from commit 9734a92) Signed-off-by: Shixiong Zhu <[email protected]>

…treaming job ## What changes were proposed in this pull request? [SPARK-19779](https://issues.apache.org/jira/browse/SPARK-19779) The PR (#17012) can to fix restart a Structured Streaming application using hdfs as fileSystem, but also exist a problem that a tmp file of delta file is still reserved in hdfs. And Structured Streaming don't delete the tmp file generated when restart streaming job in future. ## How was this patch tested? unit tests Author: guifeng <[email protected]> Closes #17124 from gf53520/SPARK-19779. (cherry picked from commit e24f21b) Signed-off-by: Shixiong Zhu <[email protected]>

…treaming job ## What changes were proposed in this pull request? [SPARK-19779](https://issues.apache.org/jira/browse/SPARK-19779) The PR (#17012) can to fix restart a Structured Streaming application using hdfs as fileSystem, but also exist a problem that a tmp file of delta file is still reserved in hdfs. And Structured Streaming don't delete the tmp file generated when restart streaming job in future. ## How was this patch tested? unit tests Author: guifeng <[email protected]> Closes #17124 from gf53520/SPARK-19779.

vitillo force-pushed the fix_rename branch from c985f39 to 9e259cb Compare February 21, 2017 08:59

vitillo force-pushed the fix_rename branch from 9e259cb to 707861a Compare February 22, 2017 09:35

zsxwing reviewed Feb 25, 2017

View reviewed changes

vitillo force-pushed the fix_rename branch 2 times, most recently from 7d44680 to 530c027 Compare February 27, 2017 14:54

zsxwing reviewed Feb 28, 2017

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreSuite.scala Outdated

Copy link

Member

zsxwing Feb 28, 2017

Choose a reason for hiding this comment

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

nit: indent

vitillo changed the title ~~[SPARK-19677][SS] Renaming a file atop an existing one should not fail on HDFS~~ [SPARK-19677][SS] Committing a delta file atop an existing one should not fail on HDFS Feb 28, 2017

Committing a delta file atop an existing one should not fail on HDFS

de228a4

vitillo force-pushed the fix_rename branch from 530c027 to de228a4 Compare February 28, 2017 13:33

asfgit closed this in 9734a92 Feb 28, 2017

zsxwing mentioned this pull request Feb 28, 2017

[SPARK-19617][SS]fix structured streaming restart bug #16980

Closed

gf53520 mentioned this pull request Mar 1, 2017

[SPARK-19779][SS]Delete needless tmp file after restart structured streaming job #17124

Closed

[SPARK-19677][SS] Committing a delta file atop an existing one should not fail on HDFS #17012

[SPARK-19677][SS] Committing a delta file atop an existing one should not fail on HDFS #17012

Uh oh!

Conversation

vitillo commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

vitillo commented Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zsxwing commented Feb 22, 2017

Uh oh!

vitillo commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitillo commented Feb 23, 2017

Uh oh!

hejix commented Feb 24, 2017

Uh oh!

zsxwing Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitillo Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

hejix commented Feb 28, 2017

Uh oh!

mridulm commented Feb 28, 2017

Uh oh!

zsxwing commented Feb 28, 2017

Uh oh!

zsxwing commented Feb 28, 2017

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

zsxwing Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

zsxwing Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Feb 28, 2017

Uh oh!

zsxwing Feb 28, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 28, 2017

Uh oh!

zsxwing commented Feb 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vitillo commented Feb 21, 2017 •

edited

Loading

vitillo commented Feb 21, 2017 •

edited

Loading

vitillo commented Feb 22, 2017 •

edited

Loading

zsxwing Feb 25, 2017 •

edited

Loading