-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-30915][SS] CompactibleFileStreamLog: Avoid reading the metadata log file when finding the latest batch ID #27664
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 3 commits
0cd8cda
d270961
30338fb
ff9078e
f6078bb
83451c1
4a5679e
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -142,7 +142,7 @@ class FileStreamSink( | |||||
| } | ||||||
|
|
||||||
| override def addBatch(batchId: Long, data: DataFrame): Unit = { | ||||||
| if (batchId <= fileLog.getLatest().map(_._1).getOrElse(-1L)) { | ||||||
|
Member
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. Nice catch! Line 196 in 7ad6ba3
spark/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/FileStreamSourceLog.scala Line 99 in 7ad6ba3
Can these two places also be optimized in this way?
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 I think so. Nice finding. Thanks! |
||||||
| if (batchId <= fileLog.getLatestBatchId().getOrElse(-1L)) { | ||||||
| logInfo(s"Skipping already committed batch $batchId") | ||||||
| } else { | ||||||
| val committer = FileCommitProtocol.instantiate( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,14 @@ | |
| package org.apache.spark.sql.execution.streaming | ||
|
|
||
| import java.io.{ByteArrayInputStream, ByteArrayOutputStream} | ||
| import java.net.URI | ||
| import java.nio.charset.StandardCharsets.UTF_8 | ||
| import java.util.concurrent.atomic.AtomicLong | ||
|
|
||
| import scala.collection.mutable | ||
| import scala.util.Random | ||
|
|
||
| import org.apache.hadoop.fs.{FSDataInputStream, Path, RawLocalFileSystem} | ||
|
|
||
| import org.apache.spark.SparkFunSuite | ||
| import org.apache.spark.sql.internal.SQLConf | ||
|
|
@@ -240,6 +247,44 @@ class FileStreamSinkLogSuite extends SparkFunSuite with SharedSparkSession { | |
| )) | ||
| } | ||
|
|
||
| test("getLatestBatchId") { | ||
|
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 don't add E2E test to simplify the test code, but if we prefer E2E than I'll try to add a new test to FileStreamSinkSuite. |
||
| withCountOpenLocalFileSystemAsLocalFileSystem { | ||
| val scheme = CountOpenLocalFileSystem.scheme | ||
| withSQLConf(SQLConf.FILE_SINK_LOG_COMPACT_INTERVAL.key -> "3") { | ||
| withTempDir { dir => | ||
| val sinkLog = new FileStreamSinkLog(FileStreamSinkLog.VERSION, spark, | ||
| s"$scheme:///${dir.getCanonicalPath}") | ||
| for (batchId <- 0L to 2L) { | ||
| sinkLog.add( | ||
| batchId, | ||
| Array(newFakeSinkFileStatus("/a/b/" + batchId, FileStreamSinkLog.ADD_ACTION))) | ||
| } | ||
|
|
||
| def getCountForOpenOnMetadataFile(batchId: Long): Long = { | ||
| val path = sinkLog.batchIdToPath(batchId).toUri.getPath | ||
| CountOpenLocalFileSystem.pathToNumOpenCalled.get(path).map(_.get()).getOrElse(0L) | ||
| } | ||
|
|
||
| CountOpenLocalFileSystem.resetCount() | ||
|
|
||
| assert(sinkLog.getLatestBatchId() === Some(2L)) | ||
| // getLatestBatchId doesn't open the latest metadata log file | ||
| (0L to 2L).foreach { batchId => | ||
| assert(getCountForOpenOnMetadataFile(batchId) === 0L) | ||
| } | ||
|
|
||
| assert(sinkLog.getLatest().map(_._1).getOrElse(-1L) === 2L) | ||
| (0L to 1L).foreach { batchId => | ||
| assert(getCountForOpenOnMetadataFile(batchId) === 0L) | ||
| } | ||
| // getLatest opens the latest metadata log file, which explains the needs on | ||
| // having "getLatestBatchId". | ||
| assert(getCountForOpenOnMetadataFile(2L) === 1L) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a fake SinkFileStatus using path and action. Most of tests don't care about other fields | ||
| * in SinkFileStatus. | ||
|
|
@@ -267,4 +312,40 @@ class FileStreamSinkLogSuite extends SparkFunSuite with SharedSparkSession { | |
| val log = new FileStreamSinkLog(FileStreamSinkLog.VERSION, spark, input.toString) | ||
| log.allFiles() | ||
| } | ||
|
|
||
| private def withCountOpenLocalFileSystemAsLocalFileSystem(body: => Unit): Unit = { | ||
|
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. The code regarding FileSystem I add here is very similar with what I add in #27620. When either one gets merged, I'll rebase and deduplicate it. |
||
| val optionKey = s"fs.${CountOpenLocalFileSystem.scheme}.impl" | ||
| val originClassForLocalFileSystem = spark.conf.getOption(optionKey) | ||
| try { | ||
| spark.conf.set(optionKey, classOf[CountOpenLocalFileSystem].getName) | ||
| body | ||
| } finally { | ||
| originClassForLocalFileSystem match { | ||
| case Some(fsClazz) => spark.conf.set(optionKey, fsClazz) | ||
| case _ => spark.conf.unset(optionKey) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class CountOpenLocalFileSystem extends RawLocalFileSystem { | ||
| import CountOpenLocalFileSystem._ | ||
|
|
||
| override def getUri: URI = { | ||
| URI.create(s"$scheme:///") | ||
| } | ||
|
|
||
| override def open(f: Path, bufferSize: Int): FSDataInputStream = { | ||
| val path = f.toUri.getPath | ||
| val curVal = pathToNumOpenCalled.getOrElseUpdate(path, new AtomicLong(0)) | ||
| curVal.incrementAndGet() | ||
| super.open(f, bufferSize) | ||
| } | ||
| } | ||
|
|
||
| object CountOpenLocalFileSystem { | ||
| val scheme = s"FileStreamSinkLogSuite${math.abs(Random.nextInt)}fs" | ||
| val pathToNumOpenCalled = new mutable.HashMap[String, AtomicLong] | ||
|
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. Some reset functionality would be good to make it re-usable. This would also make
Member
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. Nit: could you use a thread-safe map in case someone may reuse this in a streaming query?
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, I feel it's safer to make the map be thread-safe. I thought it's a contradiction to assume because we allow "resetting" the count, but it can be handled with caution. |
||
|
|
||
| def resetCount(): Unit = pathToNumOpenCalled.clear() | ||
| } | ||
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 just simply remove reading file here, but as we already get batch IDs from "listing" files, it may not even need to check for existence. It won't be the outstanding latency, though.