You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@spark.apache.org by CodingCat <gi...@git.apache.org> on 2014/02/21 06:44:08 UTC
[GitHub] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
GitHub user CodingCat opened a pull request:
https://github.com/apache/incubator-spark/pull/626
[SPARK-1100] prevent Spark from overwriting directory silently and leaving dirty directory
Thanks for Diana Carroll to report this issue
the current saveAsTextFile/SequenceFile will overwrite the output directory silently if the directory already exists, this behaviour is not desirable because
1. overwriting the data silently is not user-friendly
2. if the partition number of two writing operation changed, then the output directory will contain the results generated by two runnings
My fix includes:
1. add some new APIs with a flag for users to define whether he/she wants to overwrite the directory:
if the flag is set to true, then the output directory is deleted first and then written into the new data to prevent the output directory contains results from multiple rounds of running;
if the flag is set to false, Spark will throw an exception if the output directory already exists
2. I didn't change saveNewHadoopAPI because in the new API, the overwrite flag is defined by the implementation of RecordWriter, we don't need to control that in Spark
3. changed JavaAPI part
4. default behaviour is overwriting
-----
Two questions
1. should we deprecate the old APIs without such a flag?
2. I noticed that Spark Streaming also called these APIs, I thought we don't need to change the related part in streaming? @tdas
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/CodingCat/incubator-spark SPARK-1100
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-spark/pull/626.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 #626
----
commit 2ec87a1f63b4650036691e5bf5d484aae4e6d470
Author: CodingCat <zh...@gmail.com>
Date: 2014-02-21T05:32:17Z
add new APIs to enable users define whether to overwrite the output directory
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35731089
@mridulm Thank you for telling me the standard solution, I will revise my patch today. I learnt a lot from the discussion with you in my other patches
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35703455
Typically, the way this gets done is - write to a temporary directory, taking care of multiple attempts for same partition (failure case)/multiple concurrent executions on same partition (speculative execution case) and once job is done, move to the desired destination (or delete dir if job fails) - like what mapred does for example.
(Moves are atomic NN operations).
So when output directory is "done", it is fully done : not partially/in progress/etc.
Particularly the bug mentioned - of left over files from previous jobs, etc - is just scarey !
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35842285
@pwendell the second situation can be avoided, sorry, just brain damaged......the only issue is if there is a component relies on the fact that Spark allows the overwriting the directory before~
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35761128
I run the following command
scala> val a = sc.textFile("/Users/nanzhu/code/incubator-spark/LICENSE", 4).map(line => ("a", "b"))
scala> val a = sc.textFile("/Users/nanzhu/code/incubator-spark/LICENSE", 2).map(line => ("a", "b"))
Sorry, uploaded the wrong image, look at here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/incubator-spark/pull/626#discussion_r10025532
--- Diff: core/src/main/scala/org/apache/spark/rdd/PairRDDFunctions.scala ---
@@ -603,8 +600,12 @@ class PairRDDFunctions[K: ClassTag, V: ClassTag](self: RDD[(K, V)])
val job = new NewAPIHadoopJob(conf)
job.setOutputKeyClass(keyClass)
job.setOutputValueClass(valueClass)
+
val wrappedConf = new SerializableWritable(job.getConfiguration)
NewFileOutputFormat.setOutputPath(job, new Path(path))
+ val jobFormat = outputFormatClass.newInstance
+ jobFormat.checkOutputSpecs(new JobContext(wrappedConf.value, job.getJobID))
+
--- End diff --
iirc this will fail even if there is an empty directory - is that the intention now ?
---
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35851751
I just went through the Spark Streaming document, it seems that it's safe to follow your suggestion @pwendell
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35973937
Hi, @pwendell, do you mind explaining a bit about the reason of caring about that there are part-*** in the directory?
---
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35731273
@jyotiska that would be nice!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35975684
@CodingCat don't worry about that - I was just suggesting maybe we could allow it to succeed if the output directory exists but does not have files in it.
Would you mind adding a unit test? Based on looking at the code I'm not sure exactly what happens here if the file is full. Does it just throw an exception that fails the job?
---
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35851996
but why not just preventing users from overwriting the directory, no matter whether there is part-*?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35841703
@pwendell Thanks for the comments, I also considered what you mentioned, but will that prevent other components like Spark Streaming from doing the right job? (I'm not familiar with streaming, but it seems that it will overwrite the existing directory...)
Also how to prevent the situation that the user occasionally run the job over the same directory for two times, but with different partition number (the second running has smaller value); eventually, the directory will contain the results from two runnings.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by jyotiska <gi...@git.apache.org>.
Github user jyotiska commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35704511
I think it is a good idea to add an extra flag for overwriting. If the flag is not present, Spark should throw an exception. I will see if the bug is also present in PySpark.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35701775
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35838665
OK, fixed some bugs and squashed the commits, I think it's ready for further review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35841445
Hey @CodingCat this approach has a few drawbacks. First, it will mean a pretty bad regression for some users. For instance, say that a user is calling saveAsHadoopFile(/my-dir) and that directory has some other random stuff in at as well. Previously it would have written spark files alongside the other stuff, but with this patch it will silently delete the other data and create the directory. Second, this changes the API's all over the place which we are trying not to do. Third, it's a little scary to have code in spark that's deleting HDFS directories - I'd rather make the user do it explicitly.
What if we did the following: We look in the output directory and see if there are any part-XX files in there already, and if so we throw an exception and say that the directory already has output data in it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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] incubator-spark pull request: [SPARK-1100] prevent Spark from over...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/incubator-spark/pull/626#issuecomment-35760516
@mridulm I tested that and found that it is actually not handled in Spark,
part-00000 and part-00001 are generated by the second run of the job (the partition is set to 2)
part-00002 and part-00003 are generated by the first run of (partition number of is set to 4),
I also found this when I run Spark over S3, the old file is still left over in the output directory
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. To do so, please top-post your response.
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.
---