Skip to content

[SPARK-23815][Core]Spark writer dynamic partition overwrite mode may fail to write output on multi level partition #20931

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

Closed
fangshil wants to merge 4 commits into from
Closed

[SPARK-23815][Core]Spark writer dynamic partition overwrite mode may fail to write output on multi level partition #20931

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da63c17
Spark writer dynamic partition overwrite mode may fail to write outpu…
Mar 28, 2018
686a404
[SPARK-23815]follow up. add comment
Apr 7, 2018
a9342cc
[SPARK-23815]follow up. improve comment
Apr 7, 2018
08b9601
[SPARK-23815]follow up. further improve comment
Apr 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion core/src/main/scala/org/apache/spark/internal/io/HadoopMapReduceCommitProtocol.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,17 @@ class HadoopMapReduceCommitProtocol(
logDebug(s"Clean up default partition directories for overwriting: $partitionPaths")
for (part <- partitionPaths) {
val finalPartPath = new Path(path, part)
fs.delete(finalPartPath, true)
if (!fs.delete(finalPartPath, true) && !fs.exists(finalPartPath.getParent)) {
Copy link
Contributor

@cloud-fan cloud-fan Apr 5, 2018

Choose a reason for hiding this comment

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

why we only create the parent dir if we fail to delete the finalPartPath?

Copy link
Author

@fangshil fangshil Apr 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@cloud-fan this is to follow the behavior of HDFS rename spec: it requires the parent to be present. If we create finalPartPath directly, then it will cause another wired behavior in rename when the dst path already exists. From the HDFS spec I shared above: " If the destination exists and is a directory, the final destination of the rename becomes the destination + the filename of the source path". We have confirmed this in our production cluster, and used the current patch to only create parent dir which follows the HDFS spec exactly,

Copy link
Contributor

@jiangxb1987 jiangxb1987 Apr 6, 2018

Choose a reason for hiding this comment

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

I think the problem here is we didn't check whether the finalPartPath exists, and we shall actually check that before rename.

Copy link
Contributor

@cloud-fan cloud-fan Apr 6, 2018

Choose a reason for hiding this comment

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

I feel the code here is not safe. We may fail to delete if finalPartPath doesn't exist, or there are some real failures. We should make sure finalPartPath doesn't exist before renaming.

Copy link
Contributor

@cloud-fan cloud-fan Apr 6, 2018

Choose a reason for hiding this comment

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

BTW we should add comments around here to explain all these stuff.

Copy link
Contributor

@jiangxb1987 jiangxb1987 Apr 6, 2018

Choose a reason for hiding this comment

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

+1 on add comments.

Copy link
Author

@fangshil fangshil Apr 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The FileSystem API spec on delete says "Code SHOULD just call delete(path, recursive) and assume the destination is no longer present". Referring to its detailed spec, the only case that we may get false from delete would be finalPartPath does not exist. Other failures should result in exception. When finalPartPath does not exist, which is an expected case, we only need action if the parent of finalPartPath does not exist because otherwise we will have problem in rename according to rename spec. Please advise if you guys think we still should double-check finalPartPath before rename. will add comment after the discussion

Copy link
Contributor

@cloud-fan cloud-fan Apr 6, 2018

Choose a reason for hiding this comment

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

ah makes sense, let's add some comment to summary these discussions.

Copy link
Author

@fangshil fangshil Apr 7, 2018

Choose a reason for hiding this comment

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

added

// According to the official hadoop FileSystem API spec, delete op should assume
// the destination is no longer present regardless of return value, thus we do not
// need to double check if finalPartPath exists before rename.
// Also in our case, based on the spec, delete returns false only when finalPartPath
// does not exist. When this happens, we need to take action if parent of finalPartPath
// also does not exist(e.g. the scenario described on SPARK-23815), because
// FileSystem API spec on rename op says the rename dest(finalPartPath) must have
// a parent that exists, otherwise we may get unexpected result on the rename.
fs.mkdirs(finalPartPath.getParent)
}
fs.rename(new Path(stagingDir, part), finalPartPath)
}
}
Expand Down