-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-28199][SS] Move Trigger implementations to Triggers.scala and avoid exposing these to the end users #24996
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
Closed
Closed
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
20059fa
[SPARK-28199][SS] Remove usage of deprecated ProcessingTime in Spark …
HeartSaVioR 2acc53a
Remove ProcessingTime entirely, move ProcessingTimeTrigger to Trigger…
HeartSaVioR bc5a3fc
Fix Mima
HeartSaVioR 12655f0
nit: rename variable to follow the type
HeartSaVioR da6fa51
Move ContinuousTrigger to Triggers with deprecating legacy, deduplica…
HeartSaVioR 027e69c
Just remove origins instead of deprecating
HeartSaVioR f42e3b5
Rename and narrow down scope
HeartSaVioR File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/ProcessingTimeTrigger.scala
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.execution.streaming | ||
|
|
||
| import java.util.concurrent.TimeUnit | ||
|
|
||
| import scala.concurrent.duration.Duration | ||
|
|
||
| import org.apache.spark.annotation.Evolving | ||
| import org.apache.spark.sql.streaming.Trigger | ||
| import org.apache.spark.unsafe.types.CalendarInterval | ||
|
|
||
| /** | ||
| * A [[Trigger]] that runs a query periodically based on the processing time. If `interval` is 0, | ||
| * the query will run as fast as possible. | ||
| */ | ||
| @Evolving | ||
| private[sql] case class ProcessingTimeTrigger(intervalMs: Long) extends Trigger { | ||
| require(intervalMs >= 0, "the interval of trigger should not be negative") | ||
| } | ||
|
|
||
| private[sql] object ProcessingTimeTrigger { | ||
| def apply(interval: String): ProcessingTimeTrigger = { | ||
| val cal = CalendarInterval.fromCaseInsensitiveString(interval) | ||
| if (cal.months > 0) { | ||
| throw new IllegalArgumentException(s"Doesn't support month or year interval: $interval") | ||
| } | ||
| new ProcessingTimeTrigger(TimeUnit.MICROSECONDS.toMillis(cal.microseconds)) | ||
| } | ||
|
|
||
| def apply(interval: Duration): ProcessingTimeTrigger = { | ||
| ProcessingTimeTrigger(interval.toMillis) | ||
| } | ||
|
|
||
| def create(interval: String): ProcessingTimeTrigger = { | ||
| apply(interval) | ||
| } | ||
|
|
||
| def create(interval: Long, unit: TimeUnit): ProcessingTimeTrigger = { | ||
| ProcessingTimeTrigger(unit.toMillis(interval)) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| package org.apache.spark.sql.execution.streaming | ||
|
|
||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.sql.streaming.ProcessingTime | ||
| import org.apache.spark.sql.streaming.{ProcessingTime, Trigger} | ||
|
||
| import org.apache.spark.util.{Clock, SystemClock} | ||
|
|
||
| trait TriggerExecutor { | ||
|
|
@@ -43,7 +43,9 @@ case class OneTimeExecutor() extends TriggerExecutor { | |
| /** | ||
| * A trigger executor that runs a batch every `intervalMs` milliseconds. | ||
| */ | ||
| case class ProcessingTimeExecutor(processingTime: ProcessingTime, clock: Clock = new SystemClock()) | ||
| case class ProcessingTimeExecutor( | ||
| processingTime: ProcessingTimeTrigger, | ||
|
||
| clock: Clock = new SystemClock()) | ||
| extends TriggerExecutor with Logging { | ||
|
|
||
| private val intervalMs = processingTime.intervalMs | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 see, if this is basically an implementation class, I wonder if it belongs in the (unfortunately named)
Triggers.scalaclass, which only now hasOneTimeTrigger, but at least is just another implementation class too? No big deal.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.
Once we decide to move this class to
Triggers.scala, I guessContinuousTriggerhas to be moved too. No big deal for me too, so please let me know which feels cleaner for you.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 think I'd move it, to rationalize
Triggers.scalaand avoid another file. It doesn't matter much. I see there isContinuousTriggertoo but I guess it belongs in the.continuoussubpackage.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.
Thanks for your voice on this. Agreed on both points, I'll leave ContinuousTrigger as it is, and move ProcessingTimeTrigger.