Add next/previous firing time to context message #80
Conversation
return all firing times
|
Hey @enragedginger - any way we can see this functionality make it back into the project? This is my first contribution so let me know what's the best way to to move forward. |
|
Hi @kvma, I tried reviewing this diff today. It looks like there's several old commits and file changes showing up as part of this PR which are already on master. Can you correct this so that the |
|
@enragedginger - thanks for taking a look - I've backed the rebase I did and should be a cleaner PR now. |
|
@enragedginger @kvma Shall this PR be merged? Had to create QuartzSchedulerExtention2 to implement nextFireTime checks. |
| /* This is a somewhat questionable test as the timing between components may not match the tick off. */ | ||
| val receipt = probe.receiveWhile(Duration(1, MINUTES), Duration(15, SECONDS), 5) { | ||
| case TockWithFireTime(scheduledFireTime) => | ||
| scheduledFireTime | ||
| } | ||
| 0 until 5 foreach { i => | ||
| assert(receipt(i)==jobDt.getTime+i*10*1000) | ||
| assert(receipt(i) === jobDt.getTime + i * 10 * 1000 +- tickTolerance) |
There was a problem hiding this comment.
What's the reason for adding tickTolerance here? Was this test flaky?
There was a problem hiding this comment.
Exactly, the test was flakey. This was just a pattern that I had used in the past that helped isolate functional issues with performance issues.
| @@ -4,6 +4,19 @@ import java.util.Date | |||
| /** | |||
| * wrap msg with scheduledFireTime | |||
| */ | |||
| final case class MessageWithFireTime(msg: AnyRef, scheduledFireTime:Date) | |||
There was a problem hiding this comment.
Instead of breaking this out into several traits / classes, what are your thoughts on just changing the definition to this:
final case class MessageWithFireTimes(msg: AnyRef, scheduledFireTime: Date, previousFiringTime: Option[Date] = None, nextFiringTime: Option[Date] = None) and then always setting all three of those fields?
This should give us all of the same benefits of your initial commits while preserving backwards compatibility too.
EDIT:
Just to clarify, then we can get rid of the extra traits / classes added below.
There was a problem hiding this comment.
absolutely - the thinking here was being conservative to maintain API compatibility to the preserve the original class MessageWithFireTime. I'm happy to flatten it, but i felt it was more intrusive as my first contribution :)
|
I've merge this. However, I'm going to refactor it with the |
Changes include: