Change Piezo UI to Prevent Job Trigger Periods that are Longer than the Monitoring Period #106
Conversation
| @@ -119,6 +124,11 @@ class TriggerFormHelper(scheduler: Scheduler) extends JobDataHelper { | |||
| } | |||
| } | |||
|
|
|||
| def greaterThan(greaterThanValue: Int): Constraint[Int] = Constraint[Int]("constraints.greaterThan") { value => | |||
There was a problem hiding this comment.
Isn't this the same as Constraints.min?
There was a problem hiding this comment.
Nice catch - I'll change that
| }).verifying("Max time between successes must be greater than 0", fields => { | ||
| fields._3 > 0 | ||
| }) | ||
| "triggerMaxErrorTime" -> of(MaxSecondsBetweenSuccessesFormatter).verifying(greaterThan(0)), |
There was a problem hiding this comment.
| "triggerMaxErrorTime" -> of(MaxSecondsBetweenSuccessesFormatter).verifying(greaterThan(0)), | |
| "triggerMaxErrorTime" -> of(MaxSecondsBetweenSuccessesFormatter).verifying(Constraints.min(0)), |
| fields => { | ||
| fields._3 > 0 | ||
| }, | ||
| ), |
There was a problem hiding this comment.
I think, rather than creating a custom Formatter that re-parses sibling fields, it would be better to just add a .verifying to the form Mapping as a whole, that verifies that the "triggerMaxErrorTime" is acceptable. Something like:
| ), | |
| ) | |
| .verifying(Constraint("minTriggerMaxErrorTime") { case (trigger, _, triggerMaxErrorTime) => | |
| val maxInterval = trigger match { | |
| case t: CronTrigger => CronHelper.getMaxInterval(new CronExpression(t.getCronExpression)) | |
| case t: SimpleTrigger => t.getRepeatInterval / 1000 | |
| case _ => 0 // unrecognized trigger type, ignore interval | |
| } | |
| if (triggerMaxErrorTime > maxInterval) { | |
| Valid | |
| } else { | |
| Invalid(s"triggerMaxErrorTime must be greater than maximum trigger interval ($maxInterval seconds)") | |
| } | |
| }), |
There was a problem hiding this comment.
I had it written this way originally. The only downside is that the error message is at the top of the form instead of right beneath the field that actually needs to be changed. I feel like it's easier to read if the error message is close to what needs to be changed. Do you know of another way to get the error message to appear under the field?
There was a problem hiding this comment.
hmm, that's unfortunate. I'm not sure.
There was a problem hiding this comment.
I wonder if you specify "triggerMaxErrorTime" as the name of the constraint, if it will apply to that field?
There was a problem hiding this comment.
This didn't end up working. Would you be ok if I left it the way I have it now?
| * @param cronExpression | ||
| */ | ||
| def getMaxInterval(cronExpression: CronExpression): Long = getMaxInterval(cronExpression, None) | ||
| def getMaxInterval(cronExpression: CronExpression, timeZone: Option[TimeZone]): Long = { |
There was a problem hiding this comment.
could we have the basic function be defined as
| def getMaxInterval(cronExpression: CronExpression, timeZone: Option[TimeZone]): Long = { | |
| def getMaxInterval(cronExpression: String, timeZone: Option[TimeZone]): Long = { |
So that we don't have to unnecessrily convert a string to a cron expression, then back to a string? Having a helper that takes a CronExpression and calls getCronExpression wouldn't be bad, but havint something that operates directly on the string would probably be better.
There was a problem hiding this comment.
Yeah I think that's fine. The reason I did this was to verify that the input to getMaxInterval is a valid cron expression.
| * seconds are in each unit. "Marked units" describe when the trigger is set to fire. "Intervals" describe the number of | ||
| * units between (but not including) marked units. | ||
| */ | ||
| case class CronContainer(str: String, unitType: UnitType, markedUnits: List[Long], timeZone: TimeZone) { |
There was a problem hiding this comment.
this probably doesn't need to be a case class
| * Determines the max interval by deduction. Is only used when seconds, minutes, and hours are specified. If days, | ||
| * months, or years are specified, use getComplexMaxInterval instead. | ||
| */ | ||
| private def getSimpleMaxInterval(subexpressions: Array[String], timeZone: Option[TimeZone]): Long = { |
There was a problem hiding this comment.
this function is kind of hard to follow
There was a problem hiding this comment.
That's fair. I tried to add comments throughout the file to enhance readability. Maybe I can add some more comments in here?
| val simplifiedComplexCron = everythingElse.mkString(s"0 0 ", " ", "") | ||
| val selectedTimeZone = timeZone.getOrElse(TimeZone.getDefault) | ||
| val cronSchedule = CronScheduleBuilder.cronSchedule(simplifiedComplexCron).inTimeZone(selectedTimeZone) | ||
| val dummyTrigger: CronTrigger = TriggerBuilder.newTrigger().withSchedule(cronSchedule).build() |
There was a problem hiding this comment.
you don't need a full trigger. You can just create a CronExpression and call getNextValidTimeAfter to get the samples.
| /** | ||
| * Estimates the max interval for a given cron expression. The more samples, the better the estimate. | ||
| */ | ||
| private def getSampledMaxInterval(prev: Date, numSamples: Long, trigger: CronTrigger, maxInterval: Long = 0): Long = { |
There was a problem hiding this comment.
you could mark this as @tailrec
| lazy val wrapIntervalInSeconds: Long = unitsToSeconds(wrapInterval) | ||
|
|
||
| // the max interval is the longest interval between any two marked units in the container, including the wrap interval | ||
| lazy val maxIntervalInSeconds: Long = { |
There was a problem hiding this comment.
This currently iterates over every value. That is sub-optimal in many common cases. In particular if the expression is "*", then we can just use the duration of a single unit. If it is a range, then we just need the time between the two ends of the range. If it is an interval, then it is just the interval, possibly accounting for start time. For more complex cases, which you don't currently handle at all, it is still possible to just deal with the gaps, instead of each possible time it triggers.
There was a problem hiding this comment.
That's fair. I can make it more efficient, although we're never iterating over more than 60 items.
There was a problem hiding this comment.
For more complex cases, which you don't currently handle at all, it is still possible to just deal with the gaps, instead of each possible time it triggers.
What do you mean by this? Wouldn't I have to iterate over all of the values to determine the gaps? What complex cases am I not handling?
There was a problem hiding this comment.
I mention it in another comment, but things like 0-10,30-40 or 10-40/5
| } | ||
|
|
||
| /** | ||
| * Determines the max interval by deduction. Is only used when seconds, minutes, and hours are specified. If days, |
There was a problem hiding this comment.
I think an approach that would be simpler would be to just use your "sampling" method in all cases, and dynamically determine the number how long to sample for. Basically, find the rightmost/largest unit that isn't * or ?, then iterate over every trigger in the range of that unit. For example, if the largest non-wildcard unit is hours, then get ever trigger for the next day and find the largest interval. There would be some situations where this might perform worse than your current deductive logic, like if you had it set to run every second on every other hour of the day, but I think those would be unusual, and we don't really need to worry about them. And it is less likely to run into weird edge cases, and you don't have to worry about more complicated expressions, because the CronExpression class will take care of that for you. It also insures that your logic for parsing the cron expresssion is more in-sync with how piezo actually handles it.
Here is what I think you would need to run for for each unit:
| Largest specificified unit | how long to get samples for |
|---|---|
| seconds | 1 minute |
| minutes | 1 hour |
| hours | 1 day (maybe also run over leap days, if dealing with time zones) |
| day of month | a little over a month, but make sure to run during a 31 day month rather than current time. May need to handle "W" values specially, since that depends on the month, maybe just add 2 to the result in that case? |
| month | 1 year |
| day of week | 7 days normally (note you can't have both day of month and day of week). But if the # mode is used, we'll need to do at least a month, probably more since the exact duration depends on the months involved. Maybe add a buffer of 1 week in that case? |
| year | this would be pretty unusual, but I guess run from now until 2199? or use some deduction for this one? |
There was a problem hiding this comment.
I think there's a lot of great ideas here. I essentially took a lot of what you said and mixed it with what I had. I now use sampling for each of the subexpressions (seconds, minutes, hours, days). I also group day of month, month, day of week, and year altogether as "days" since all of these can affect the cadence at which days fire.
| maxSecondsBetweenSuccesses <- parsing(_.toInt, "Numeric value expected", Nil)(key, data) | ||
| maxIntervalTime <- { | ||
| if (data.contains("cron.cronExpression")) { | ||
| parsing(expr => CronHelper.getMaxInterval(expr), "try again.", Nil)( |
There was a problem hiding this comment.
should we use better error messages for these?
There was a problem hiding this comment.
"try again" is appended to the error message that is generated from the parsing method.
This is what it looks like in practice:
| */ | ||
| def getMaxInterval(cronExpression: String): Long = { | ||
| try { | ||
| val (secondsMinutesHourStrings, dayStrings) = cronExpression.split("\\s").splitAt(3) |
There was a problem hiding this comment.
| val (secondsMinutesHourStrings, dayStrings) = cronExpression.split("\\s").splitAt(3) | |
| val (secondsMinutesHourStrings, dayStrings) = cronExpression.split("\\s+").splitAt(3) |
I think we want to split by any amount of whitespace.
| if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) IMPOSSIBLE_MAX_INTERVAL | ||
| else { |
There was a problem hiding this comment.
style:
| if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) IMPOSSIBLE_MAX_INTERVAL | |
| else { | |
| if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) { | |
| IMPOSSIBLE_MAX_INTERVAL | |
| } else { |
| val outermost = subexpressions(outermostIndex) | ||
| if (outermost.maxInterval == IMPOSSIBLE_MAX_INTERVAL) IMPOSSIBLE_MAX_INTERVAL | ||
| else { | ||
| val innermost = subexpressions.slice(outermostIndex + 1, subexpressions.size) |
There was a problem hiding this comment.
maybe just inner, since this is all subexpressions more inner than the outermost one. Not just the innermost one.
| extends Subexpression(str, secondsPerUnit) { | ||
|
|
||
| final protected val format = new SimpleDateFormat("MMMM d, yyyy", Locale.ENGLISH) | ||
| final protected val startInstant: Instant = format.parse("September 3, 2010").toInstant.minus(1, ChronoUnit.SECONDS) |
There was a problem hiding this comment.
| final protected val startInstant: Instant = format.parse("September 3, 2010").toInstant.minus(1, ChronoUnit.SECONDS) | |
| final protected val startInstant: Instant = LocalDate.of(2010, Month.SEPTEMBER, 3).atStartOfDay.toInstant(ZoneOffset.UTC) |
And then I don't think you need the SimpleDateFormat. Also, this could be put in associated object, so that it is only created once, instead of once for each instance of this class.
|
|
||
| final protected val format = new SimpleDateFormat("MMMM d, yyyy", Locale.ENGLISH) | ||
| final protected val startInstant: Instant = format.parse("September 3, 2010").toInstant.minus(1, ChronoUnit.SECONDS) | ||
| final override protected val startDate = new Date(startInstant.toEpochMilli) |
There was a problem hiding this comment.
Why are we using Date here instead of keeping it as an instant?
There was a problem hiding this comment.
And if you do really need a Date for some reason, this could be
| final override protected val startDate = new Date(startInstant.toEpochMilli) | |
| final override protected val startDate = Date.from(startInstant) |
There was a problem hiding this comment.
ah, we have to interface with the quartz API which still uses Date 😢
I guess using a Date is probably simplest.
| final protected val format = new SimpleDateFormat("MMMM d, yyyy", Locale.ENGLISH) | ||
| final protected val startInstant: Instant = format.parse("September 3, 2010").toInstant.minus(1, ChronoUnit.SECONDS) | ||
| final override protected val startDate = new Date(startInstant.toEpochMilli) | ||
| final protected val endDate = new Date( |
There was a problem hiding this comment.
again, this could probably just stay as an Instant
| final protected val startInstant: Instant = format.parse("September 3, 2010").toInstant.minus(1, ChronoUnit.SECONDS) | ||
| final override protected val startDate = new Date(startInstant.toEpochMilli) | ||
| final protected val endDate = new Date( | ||
| startInstant.plus(numUnitsInContainer * secondsPerUnit, ChronoUnit.SECONDS).toEpochMilli, |
There was a problem hiding this comment.
instead of having secondsPerUint as the parameter, you could have a parameter of ChronoUnit, and this would become
| startInstant.plus(numUnitsInContainer * secondsPerUnit, ChronoUnit.SECONDS).toEpochMilli, | |
| startInstant.plus(numUnitsInContainer, timeUnit).toEpochMilli, |
| private def getMaxInterval(expr: CronExpression, prev: Date, end: Date, maxInterval: Long): Long = { | ||
| Option(expr.getTimeAfter(prev)) match { | ||
| case Some(curr) if !prev.after(end) => // iterate once past the "end" in order to wrap around | ||
| val currentInterval = curr.toInstant.getEpochSecond - prev.toInstant.getEpochSecond |
There was a problem hiding this comment.
| val currentInterval = curr.toInstant.getEpochSecond - prev.toInstant.getEpochSecond | |
| val currentInterval = (curr.getTime - prev.getTime) / 1000 |
No need to convert to Instant just to get the timestamp
| } | ||
|
|
||
| @tailrec | ||
| private def getLastTriggerTime(expr: CronExpression, prev: Date, end: Date): Long = { |
There was a problem hiding this comment.
It's too bad CronExpresion.getTimeBefore is "NOT YET IMPLEMENTED"
There was a problem hiding this comment.
the current implementation will have to make two passes over all of the values. It would be possible to do a single pass that accumulates both the max interval and the last trigger time. I'm not sure if it is worth making the code more complicated though.
There was a problem hiding this comment.
True. I traded some optimization for readability here
| private def getSampledMaxInterval(prev: Date, numSamples: Long, expr: CronExpression, maxInterval: Long = 0): Long = { | ||
| Option(expr.getTimeAfter(prev)) match { | ||
| case Some(next) if numSamples > 0 => | ||
| val intervalInSeconds = next.toInstant.getEpochSecond - prev.toInstant.getEpochSecond |
There was a problem hiding this comment.
| val intervalInSeconds = next.toInstant.getEpochSecond - prev.toInstant.getEpochSecond | |
| val intervalInSeconds = (next.getTime - prev.getTilme) / 1000 |
mattspataro
force-pushed
the
guardrails-for-monitoring
branch
from
06b912e
to
6b9e151
Compare
See this PROD issue: https://lucidatlassian.atlassian.net/browse/PROD-2269
It should now be impossible to change a job trigger schedule such that the maximum schedule period is larger than the monitoring window. Additionally, it should be impossible to change the monitoring window such that it’s shorter than the maximum schedule period.