-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-20568][SS] Provide option to clean up completed files in streaming query #22952
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.
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
Changes from 9 commits
30b82df
1697c86
2f5a73f
6e2a824
33a5331
b1a6bec
b67778a
dd9d4ad
178d2f4
21c71c4
01f5750
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,9 @@ package org.apache.spark.sql.execution.streaming | |
| import java.net.URI | ||
| import java.util.concurrent.TimeUnit._ | ||
|
|
||
| import org.apache.hadoop.fs.{FileStatus, Path} | ||
| import scala.util.control.NonFatal | ||
|
|
||
| import org.apache.hadoop.fs.{FileStatus, FileSystem, GlobFilter, Path} | ||
|
|
||
| import org.apache.spark.deploy.SparkHadoopUtil | ||
| import org.apache.spark.internal.Logging | ||
|
|
@@ -53,6 +55,18 @@ class FileStreamSource( | |
| fs.makeQualified(new Path(path)) // can contain glob patterns | ||
| } | ||
|
|
||
| private val sourceCleaner: FileStreamSourceCleaner = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't going to ask, but since I have more comments... I think it's better if this were an Similarly, below, you'll resolve the (I'm almost suggesting that there should be a separate implementation for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's a good suggestion. Will address. |
||
| val (archiveFs, qualifiedArchivePath) = sourceOptions.sourceArchiveDir match { | ||
| case Some(dir) => | ||
| val path = new Path(dir) | ||
| val fs = path.getFileSystem(hadoopConf) | ||
| (Some(fs), Some(fs.makeQualified(path))) | ||
|
|
||
| case None => (None, None) | ||
| } | ||
| new FileStreamSourceCleaner(fs, qualifiedBasePath, archiveFs, qualifiedArchivePath) | ||
| } | ||
|
|
||
| private val optionsWithPartitionBasePath = sourceOptions.optionMapWithoutPath ++ { | ||
| if (!SparkHadoopUtil.get.isGlobPath(new Path(path)) && options.contains("path")) { | ||
| Map("basePath" -> path) | ||
|
|
@@ -237,6 +251,7 @@ class FileStreamSource( | |
| val files = allFiles.sortBy(_.getModificationTime)(fileSortOrder).map { status => | ||
| (status.getPath.toUri.toString, status.getModificationTime) | ||
| } | ||
|
|
||
| val endTime = System.nanoTime | ||
| val listingTimeMs = NANOSECONDS.toMillis(endTime - startTime) | ||
| if (listingTimeMs > 2000) { | ||
|
|
@@ -258,16 +273,33 @@ class FileStreamSource( | |
| * equal to `end` and will only request offsets greater than `end` in the future. | ||
| */ | ||
| override def commit(end: Offset): Unit = { | ||
| // No-op for now; FileStreamSource currently garbage-collects files based on timestamp | ||
| // and the value of the maxFileAge parameter. | ||
| val logOffset = FileStreamSourceOffset(end).logOffset | ||
|
|
||
| if (sourceOptions.cleanSource != CleanSourceMode.OFF) { | ||
| val files = metadataLog.get(Some(logOffset), Some(logOffset)).flatMap(_._2) | ||
| val validFileEntities = files.filter(_.batchId == logOffset) | ||
| logDebug(s"completed file entries: ${validFileEntities.mkString(",")}") | ||
| sourceOptions.cleanSource match { | ||
| case CleanSourceMode.ARCHIVE => | ||
| validFileEntities.foreach(sourceCleaner.archive) | ||
|
|
||
| case CleanSourceMode.DELETE => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the delete operation update the commit to remove the file as well if possible? Asking because if a file gets deleted and then another file with the same name was uploaded, it wont get processed.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... not sure how it works with current file stream source. I'd rather keep compatibility with current behavior so I need to check it first.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without this change the file which uploaded with the same name not get processed again. It may cause issues with exactly once...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I don't touch the mechanism how file source deals with files, which means it doesn't change the behavior. (Please correct me if I'm missing here.) That's why I need to check how current file stream source deals with new file with same name. If the behavior of current master and this patch are same and it doesn't work as we expect/want, addressing it is beyond this PR and better to be filed and addressed separately. Another thing to note is, we need to also consider that some options like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HeartSaVioR reading back my comment from yesterday is a bit... well not really clean :)
references the same thing what you've mentioned on #23782 and agree with you. Reprocessing files with the same name may cause several issues. What if file being archived and some producer is trying to append it, etc... |
||
| validFileEntities.foreach(sourceCleaner.delete) | ||
|
|
||
| case _ => | ||
| } | ||
| } else { | ||
| // No-op for now; FileStreamSource currently garbage-collects files based on timestamp | ||
| // and the value of the maxFileAge parameter. | ||
| } | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can also purge metadata here to prevent growing infinitely given that Spark guarantees it will not happen to request offset equal or less than |
||
| } | ||
|
|
||
| override def stop(): Unit = {} | ||
| } | ||
|
|
||
|
|
||
| object FileStreamSource { | ||
|
|
||
| /** Timestamp for file modification time, in ms since January 1, 1970 UTC. */ | ||
| type Timestamp = Long | ||
|
|
||
|
|
@@ -330,4 +362,77 @@ object FileStreamSource { | |
|
|
||
| def size: Int = map.size() | ||
| } | ||
|
|
||
| private[sql] class FileStreamSourceCleaner( | ||
| fileSystem: FileSystem, | ||
| sourcePath: Path, | ||
| baseArchiveFileSystem: Option[FileSystem], | ||
| baseArchivePath: Option[Path]) extends Logging { | ||
| assertParameters() | ||
|
|
||
| private def assertParameters(): Unit = { | ||
| require(baseArchiveFileSystem.isDefined == baseArchivePath.isDefined) | ||
|
|
||
| baseArchiveFileSystem.foreach { fs => | ||
| require(fileSystem.getUri == fs.getUri, "Base archive path is located on a different " + | ||
| s"file system than the source files. source path: $sourcePath" + | ||
| s" / base archive path: ${baseArchivePath.get}") | ||
| } | ||
|
|
||
| baseArchivePath.foreach { path => | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove |
||
| /** | ||
| * FileStreamSource reads the files which one of below conditions is met: | ||
| * 1) file itself is matched with source path | ||
| * 2) parent directory is matched with source path | ||
| * | ||
| * Checking with glob pattern is costly, so set this requirement to eliminate the cases | ||
| * where the archive path can be matched with source path. For example, when file is moved | ||
| * to archive directory, destination path will retain input file's path as suffix, so | ||
| * destination path can't be matched with source path if archive directory's depth is longer | ||
| * than 2, as neither file nor parent directory of destination path can be matched with | ||
| * source path. | ||
| */ | ||
| require(path.depth() > 2, "Base archive path must have a depth of at least 2 " + | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check says "greater than 2" but the error says "at least 2". Which one is right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the explanation says about 2 "subdirectories", not 2 "depth". / denotes its own depth. I don't think depth is the term end users are familiar with - I'll remove the part "a depth of". |
||
| "subdirectories. e.g. '/data/archive'") | ||
| } | ||
| } | ||
|
|
||
| def archive(entry: FileEntry): Unit = { | ||
| require(baseArchivePath.isDefined) | ||
|
|
||
| val curPath = new Path(new URI(entry.path)) | ||
| val newPath = new Path(baseArchivePath.get.toString.stripSuffix("/") + curPath.toUri.getPath) | ||
|
|
||
| try { | ||
| logDebug(s"Creating directory if it doesn't exist ${newPath.getParent}") | ||
| if (!fileSystem.exists(newPath.getParent)) { | ||
| fileSystem.mkdirs(newPath.getParent) | ||
| } | ||
|
|
||
| logDebug(s"Archiving completed file $curPath to $newPath") | ||
| if (!fileSystem.rename(curPath, newPath)) { | ||
| logWarning(s"Fail to move $curPath to $newPath / skip moving file.") | ||
| } | ||
| } catch { | ||
| case NonFatal(e) => | ||
| logWarning(s"Fail to move $curPath to $newPath / skip moving file.", e) | ||
| } | ||
| } | ||
|
|
||
| def delete(entry: FileEntry): Unit = { | ||
| val curPath = new Path(new URI(entry.path)) | ||
| try { | ||
| logDebug(s"Removing completed file $curPath") | ||
|
|
||
| if (!fileSystem.delete(curPath, false)) { | ||
| logWarning(s"Failed to remove $curPath / skip removing file.") | ||
| } | ||
| } catch { | ||
| case NonFatal(e) => | ||
| // Log to error but swallow exception to avoid process being stopped | ||
| logWarning(s"Fail to remove $curPath / skip removing file.", e) | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop s3n & s3 refs as they have gone from deprecated to deceased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like beyond of this PR: we can address it in separate PR. Could you raise another one?