-
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 2 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 |
|---|---|---|
|
|
@@ -71,7 +71,10 @@ object OffsetSeq { | |
| * @param batchTimestampMs: The current batch processing timestamp. | ||
| * Time unit: milliseconds | ||
| */ | ||
| case class OffsetSeqMetadata(var batchWatermarkMs: Long = 0, var batchTimestampMs: Long = 0) { | ||
| case class OffsetSeqMetadata( | ||
| var batchWatermarkMs: Long = 0, | ||
| var batchTimestampMs: Long = 0, | ||
| var numShufflePartitions: Int = 0) { | ||
|
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. It's better to use
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. Hi @kunalkhamar, in case you would update
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. @zsxwing Changed to a map
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. @lw-lin Hi Liwei! |
||
| def json: String = Serialization.write(this)(OffsetSeqMetadata.format) | ||
| } | ||
|
|
||
|
|
||
| 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,8 @@ class StreamExecution( | |
| } | ||
|
|
||
| /** Metadata associated with the offset seq of a batch in the query. */ | ||
| protected var offsetSeqMetadata = OffsetSeqMetadata() | ||
| protected var offsetSeqMetadata = | ||
| OffsetSeqMetadata(numShufflePartitions = sparkSession.conf.get(SQLConf.SHUFFLE_PARTITIONS)) | ||
|
|
||
| override val id: UUID = UUID.fromString(streamMetadata.id) | ||
|
|
||
|
|
@@ -380,7 +382,20 @@ class StreamExecution( | |
| logInfo(s"Resuming streaming query, starting with batch $batchId") | ||
| currentBatchId = batchId | ||
| availableOffsets = nextOffsets.toStreamProgress(sources) | ||
| offsetSeqMetadata = nextOffsets.metadata.getOrElse(OffsetSeqMetadata()) | ||
| val numShufflePartitionsFromConf = sparkSession.conf.get(SQLConf.SHUFFLE_PARTITIONS) | ||
| offsetSeqMetadata = nextOffsets | ||
| .metadata | ||
| .getOrElse(OffsetSeqMetadata(0, 0, numShufflePartitionsFromConf)) | ||
|
|
||
| /* | ||
| * For backwards compatibility, if # partitions was not recorded in the offset log, then | ||
| * ensure it is non-zero. The new value is picked up from the conf. | ||
| */ | ||
|
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. for inline comment with the code, use // and not /* .. */.
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. |
||
| if (offsetSeqMetadata.numShufflePartitions == 0) { | ||
| offsetSeqMetadata.numShufflePartitions = numShufflePartitionsFromConf | ||
| logDebug("Number of shuffle partitions from previous run not found in checkpoint data. " + | ||
| s"Using the value from the conf, $numShufflePartitionsFromConf partitions.") | ||
| } | ||
| logDebug(s"Found possibly unprocessed offsets $availableOffsets " + | ||
| s"at batch timestamp ${offsetSeqMetadata.batchTimestampMs}") | ||
|
|
||
|
|
@@ -542,9 +557,16 @@ class StreamExecution( | |
| cd.dataType, cd.timeZoneId) | ||
| } | ||
|
|
||
| // Fork a cloned session and set confs to disallow change in number of partitions | ||
| val sparkSessionForCurrentBatch = sparkSession.cloneSession() | ||
| sparkSessionForCurrentBatch.conf.set( | ||
| SQLConf.SHUFFLE_PARTITIONS.key, | ||
| offsetSeqMetadata.numShufflePartitions.toString) | ||
| sparkSessionForCurrentBatch.conf.set(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key, "false") | ||
|
|
||
| reportTimeTaken("queryPlanning") { | ||
| lastExecution = new IncrementalExecution( | ||
| sparkSession, | ||
| sparkSessionForCurrentBatch, | ||
| triggerLogicalPlan, | ||
| outputMode, | ||
| checkpointFile("state"), | ||
|
|
@@ -554,7 +576,8 @@ class StreamExecution( | |
| } | ||
|
|
||
| val nextBatch = | ||
| new Dataset(sparkSession, lastExecution, RowEncoder(lastExecution.analyzed.schema)) | ||
| new Dataset(sparkSessionForCurrentBatch, lastExecution, | ||
| RowEncoder(lastExecution.analyzed.schema)) | ||
|
|
||
| reportTimeTaken("addBatch") { | ||
| sink.addBatch(currentBatchId, nextBatch) | ||
|
|
||
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.