-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-19873][SS] Record num shuffle partitions in offset log and enforce in next batch. #17216
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 7 commits
12f5fd3
9ff4d29
60ec7da
c688e84
f6bd071
3af1cb4
1cacd32
030e635
5c851a5
dfae7be
4733b4e
3ae4414
a2b32ce
3abe0a0
a0c71af
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 |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ import org.apache.spark.sql.catalyst.expressions.{Attribute, AttributeMap, Curre | |
| import org.apache.spark.sql.catalyst.plans.logical.{LocalRelation, LogicalPlan} | ||
| import org.apache.spark.sql.execution.QueryExecution | ||
| import org.apache.spark.sql.execution.command.StreamingExplainCommand | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.streaming._ | ||
| import org.apache.spark.util.{Clock, UninterruptibleThread, Utils} | ||
|
|
||
|
|
@@ -117,7 +118,9 @@ class StreamExecution( | |
| } | ||
|
|
||
| /** Metadata associated with the offset seq of a batch in the query. */ | ||
| protected var offsetSeqMetadata = OffsetSeqMetadata() | ||
| protected var offsetSeqMetadata = | ||
| OffsetSeqMetadata(conf = Map(SQLConf.SHUFFLE_PARTITIONS.key -> | ||
| sparkSession.conf.get(SQLConf.SHUFFLE_PARTITIONS).toString)) | ||
|
|
||
| override val id: UUID = UUID.fromString(streamMetadata.id) | ||
|
|
||
|
|
@@ -256,6 +259,10 @@ class StreamExecution( | |
| updateStatusMessage("Initializing sources") | ||
| // force initialization of the logical plan so that the sources can be created | ||
| logicalPlan | ||
|
|
||
| // Isolated spark session to run the batches with. | ||
| val sparkSessionToRunBatches = sparkSession.cloneSession() | ||
|
|
||
| if (state.compareAndSet(INITIALIZING, ACTIVE)) { | ||
| // Unblock `awaitInitialization` | ||
| initializationLatch.countDown() | ||
|
|
@@ -276,7 +283,7 @@ class StreamExecution( | |
| if (dataAvailable) { | ||
| currentStatus = currentStatus.copy(isDataAvailable = true) | ||
| updateStatusMessage("Processing new data") | ||
| runBatch() | ||
| runBatch(sparkSessionToRunBatches) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -380,7 +387,27 @@ class StreamExecution( | |
| logInfo(s"Resuming streaming query, starting with batch $batchId") | ||
| currentBatchId = batchId | ||
| availableOffsets = nextOffsets.toStreamProgress(sources) | ||
| offsetSeqMetadata = nextOffsets.metadata.getOrElse(OffsetSeqMetadata()) | ||
|
|
||
| // initialize metadata | ||
| val shufflePartitionsSparkSession: Int = sparkSession.conf.get(SQLConf.SHUFFLE_PARTITIONS) | ||
| offsetSeqMetadata = { | ||
| if (nextOffsets.metadata.isEmpty) { | ||
| OffsetSeqMetadata(0, 0, | ||
|
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. nit: can you make this call with named params
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. Changed. |
||
| Map(SQLConf.SHUFFLE_PARTITIONS.key -> shufflePartitionsSparkSession.toString)) | ||
| } else { | ||
| val metadata = nextOffsets.metadata.get | ||
| val shufflePartitionsToUse = metadata.conf.getOrElse(SQLConf.SHUFFLE_PARTITIONS.key, { | ||
| // For backward compatibility, if # partitions was not recorded in the offset log, | ||
| // then ensure it is not missing. The new value is picked up from the conf. | ||
| logDebug("Number of shuffle partitions from previous run not found in checkpoint. " | ||
|
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. Make this a log warning. So that we can debug. And it should be printed only once, at the time of upgrading for the first time.
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. Changed to log warning. |
||
| + s"Using the value from the conf, $shufflePartitionsSparkSession partitions.") | ||
| shufflePartitionsSparkSession | ||
| }) | ||
| OffsetSeqMetadata(metadata.batchWatermarkMs, metadata.batchTimestampMs, | ||
| metadata.conf + (SQLConf.SHUFFLE_PARTITIONS.key -> shufflePartitionsToUse.toString)) | ||
| } | ||
| } | ||
|
|
||
| logDebug(s"Found possibly unprocessed offsets $availableOffsets " + | ||
| s"at batch timestamp ${offsetSeqMetadata.batchTimestampMs}") | ||
|
|
||
|
|
@@ -498,8 +525,9 @@ class StreamExecution( | |
|
|
||
| /** | ||
| * Processes any data available between `availableOffsets` and `committedOffsets`. | ||
| * @param sparkSessionToRunBatch Isolated [[SparkSession]] to run this batch with. | ||
| */ | ||
| private def runBatch(): Unit = { | ||
| private def runBatch(sparkSessionToRunBatch: SparkSession): Unit = { | ||
| // Request unprocessed data from all sources. | ||
| newData = reportTimeTaken("getBatch") { | ||
| availableOffsets.flatMap { | ||
|
|
@@ -542,9 +570,15 @@ class StreamExecution( | |
| cd.dataType, cd.timeZoneId) | ||
| } | ||
|
|
||
| // Reset confs to disallow change in number of partitions | ||
|
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. Why need to set the confs for every batch? You can set it after recovering
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. Good point, changed. |
||
| sparkSessionToRunBatch.conf.set( | ||
| SQLConf.SHUFFLE_PARTITIONS.key, | ||
| offsetSeqMetadata.conf(SQLConf.SHUFFLE_PARTITIONS.key)) | ||
| sparkSessionToRunBatch.conf.set(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key, "false") | ||
|
|
||
| reportTimeTaken("queryPlanning") { | ||
| lastExecution = new IncrementalExecution( | ||
| sparkSession, | ||
| sparkSessionToRunBatch, | ||
| triggerLogicalPlan, | ||
| outputMode, | ||
| checkpointFile("state"), | ||
|
|
@@ -554,7 +588,7 @@ class StreamExecution( | |
| } | ||
|
|
||
| val nextBatch = | ||
| new Dataset(sparkSession, lastExecution, RowEncoder(lastExecution.analyzed.schema)) | ||
| new Dataset(sparkSessionToRunBatch, lastExecution, RowEncoder(lastExecution.analyzed.schema)) | ||
|
|
||
| reportTimeTaken("addBatch") { | ||
| sink.addBatch(currentBatchId, nextBatch) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| {"id":"dddc5e7f-1e71-454c-8362-de184444fb5a"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| v1 | ||
| {"batchWatermarkMs":0,"batchTimestampMs":1489180207737} | ||
| 0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| v1 | ||
| {"batchWatermarkMs":0,"batchTimestampMs":1489180209261} | ||
| 2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ package org.apache.spark.sql.execution.streaming | |
| import java.io.File | ||
|
|
||
| import org.apache.spark.SparkFunSuite | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.test.SharedSQLContext | ||
|
|
||
| class OffsetSeqLogSuite extends SparkFunSuite with SharedSQLContext { | ||
|
|
@@ -28,12 +29,32 @@ class OffsetSeqLogSuite extends SparkFunSuite with SharedSQLContext { | |
| case class StringOffset(override val json: String) extends Offset | ||
|
|
||
| test("OffsetSeqMetadata - deserialization") { | ||
| assert(OffsetSeqMetadata(0, 0) === OffsetSeqMetadata("""{}""")) | ||
| assert(OffsetSeqMetadata(1, 0) === OffsetSeqMetadata("""{"batchWatermarkMs":1}""")) | ||
| assert(OffsetSeqMetadata(0, 2) === OffsetSeqMetadata("""{"batchTimestampMs":2}""")) | ||
| assert( | ||
| OffsetSeqMetadata(1, 2) === | ||
| OffsetSeqMetadata("""{"batchWatermarkMs":1,"batchTimestampMs":2}""")) | ||
| val key = SQLConf.SHUFFLE_PARTITIONS.key | ||
|
|
||
| def getConfWith(shufflePartitions: Int): Map[String, String] = { | ||
| Map(key -> shufflePartitions.toString) | ||
| } | ||
|
|
||
| // None set | ||
| assert(OffsetSeqMetadata(0, 0, Map.empty) === OffsetSeqMetadata("""{}""")) | ||
|
|
||
| // One set | ||
| assert(OffsetSeqMetadata(1, 0, Map.empty) === OffsetSeqMetadata("""{"batchWatermarkMs":1}""")) | ||
| assert(OffsetSeqMetadata(0, 2, Map.empty) === OffsetSeqMetadata("""{"batchTimestampMs":2}""")) | ||
| assert(OffsetSeqMetadata(0, 0, getConfWith(shufflePartitions = 2)) === | ||
| OffsetSeqMetadata(s"""{"conf": {"$key":2}}""")) | ||
|
|
||
| // Two set | ||
| assert(OffsetSeqMetadata(1, 2, Map.empty) === | ||
| OffsetSeqMetadata("""{"batchWatermarkMs":1,"batchTimestampMs":2}""")) | ||
| assert(OffsetSeqMetadata(1, 0, getConfWith(shufflePartitions = 3)) === | ||
| OffsetSeqMetadata(s"""{"batchWatermarkMs":1,"conf": {"$key":3}}""")) | ||
| assert(OffsetSeqMetadata(0, 2, getConfWith(shufflePartitions = 3)) === | ||
| OffsetSeqMetadata(s"""{"batchTimestampMs":2,"conf": {"$key":3}}""")) | ||
|
|
||
| // All set | ||
| assert(OffsetSeqMetadata(1, 2, getConfWith(shufflePartitions = 3)) === | ||
| OffsetSeqMetadata(s"""{"batchWatermarkMs":1,"batchTimestampMs":2,"conf": {"$key":3}}""")) | ||
|
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 add a test to verify that unknown fields don't break the serialization? Such as assert(OffsetSeqMetadata(1, 2, getConfWith(shufflePartitions = 3)) ===
OffsetSeqMetadata(
s"""{"batchWatermarkMs":1,"batchTimestampMs":2,"conf": {"$key":3}},"unknown":1"""))
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. Added. |
||
| } | ||
|
|
||
| test("OffsetSeqLog - serialization - deserialization") { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ | |
|
|
||
| package org.apache.spark.sql.streaming | ||
|
|
||
| import java.io.{InterruptedIOException, IOException} | ||
| import java.io.{File, InterruptedIOException, IOException} | ||
| import java.util.concurrent.{CountDownLatch, TimeoutException, TimeUnit} | ||
|
|
||
| import scala.reflect.ClassTag | ||
|
|
@@ -28,6 +28,7 @@ import org.apache.spark.sql.catalyst.streaming.InternalOutputModes | |
| import org.apache.spark.sql.execution.command.ExplainCommand | ||
| import org.apache.spark.sql.execution.streaming._ | ||
| import org.apache.spark.sql.functions._ | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.sources.StreamSourceProvider | ||
| import org.apache.spark.sql.types.{IntegerType, StructField, StructType} | ||
|
|
||
|
|
@@ -389,6 +390,61 @@ class StreamSuite extends StreamTest { | |
| query.stop() | ||
| assert(query.exception.isEmpty) | ||
| } | ||
|
|
||
| test("SPARK-19873: streaming aggregation with change in number of partitions") { | ||
| val inputData = MemoryStream[(Int, Int)] | ||
| val agg = inputData.toDS().groupBy("_1").count() | ||
|
|
||
| testStream(agg, OutputMode.Complete())( | ||
| AddData(inputData, (1, 0), (2, 0)), | ||
| StartStream(additionalConfs = Map(SQLConf.SHUFFLE_PARTITIONS.key -> "2")), | ||
| CheckAnswer((1, 1), (2, 1)), | ||
| StopStream, | ||
| AddData(inputData, (3, 0), (2, 0)), | ||
| StartStream(additionalConfs = Map(SQLConf.SHUFFLE_PARTITIONS.key -> "5")), | ||
| CheckAnswer((1, 1), (2, 2), (3, 1)), | ||
| StopStream, | ||
| AddData(inputData, (3, 0), (1, 0)), | ||
| StartStream(additionalConfs = Map(SQLConf.SHUFFLE_PARTITIONS.key -> "1")), | ||
| CheckAnswer((1, 2), (2, 2), (3, 2))) | ||
| } | ||
|
|
||
| test("SPARK-19873: backward compat with checkpoints that do not record shuffle partitions") { | ||
| val inputData = MemoryStream[Int] | ||
| inputData.addData(1, 2, 3, 4) | ||
| inputData.addData(3, 4, 5, 6) | ||
| inputData.addData(5, 6, 7, 8) | ||
|
|
||
| val resourceUri = | ||
|
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. can you add a comment saying that start the query with existing checkpoints generated by 2.1 which do not have shuffle partitions recorded.
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. Added more comments. |
||
| this.getClass.getResource("/structured-streaming/checkpoint-version-2.1.0").toURI | ||
| val checkpointDir = new File(resourceUri).getCanonicalPath | ||
| val query = inputData | ||
| .toDF() | ||
| .groupBy($"value") | ||
| .agg(count("*")) | ||
| .writeStream | ||
| .queryName("counts") | ||
| .outputMode("complete") | ||
| .option("checkpointLocation", checkpointDir) | ||
| .format("memory") | ||
|
|
||
| // Checkpoint data was generated by a query with 10 shuffle partitions. | ||
| // Test if recovery from checkpoint is successful. | ||
| withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "10") { | ||
| query.start().processAllAvailable() | ||
|
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. its not clear that this would actually re-execute a batch. unless a batch is executed, this does not test anything. so how about you add more data after processAllAvailable(), to ensure that at least one batch is actually executed?
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. Added. |
||
|
|
||
| QueryTest.checkAnswer(spark.table("counts").toDF(), | ||
| Row("1", 1) :: Row("2", 1) :: Row("3", 2) :: Row("4", 2) :: | ||
| Row("5", 2) :: Row("6", 2) :: Row("7", 1) :: Row("8", 1) :: Nil) | ||
| } | ||
|
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. you dont seem to stop the query? would be good put a
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. Added |
||
|
|
||
| // If the number of partitions is greater, should throw exception. | ||
| withSQLConf(SQLConf.SHUFFLE_PARTITIONS.key -> "15") { | ||
|
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. can you check whether the returned message is useful?
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. Seems okay to me. Underlying cause is
|
||
| intercept[IllegalArgumentException] { | ||
| query.start().processAllAvailable() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| abstract class FakeSource extends StreamSourceProvider { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Do you know why we have this as var? Can they be made into vals.
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.
Changed to vals.