Scheduler Bugfix: getFutureActionTimes should ignore actions prior to update time, respect RemainingActions counter #6122
Conversation
This file contains 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
…prior to update time, respects RemainingActions counter
dnr
reviewed
Jun 14, 2024
dnr
approved these changes
Jun 14, 2024
stephanos
pushed a commit
that referenced
this pull request
Jun 14, 2024
… update time, respect RemainingActions counter (#6122) ## What changed/why? In #6028/#5381, our scheduler's run loop accounted for update requests that occur between nominal and actual/jittered times. The run loop will correctly skip over action times who are scheduled prior to the schedule's update time (such as when a schedule is recalculated to a new spec). This PR updates `getFutureActionTimes` (used as part of the scheduler's describe and list operations) to account for action times preceding the update time, as well as to properly reflect the number of remaining actions in a schedule. ## How did you test it? - Updated unit tests for both limited actions (`TestLimitedActions`) as well as filtered update times (`TestUpdateNotRetroactive`). `TestUpdateNotRetroactive` is the test that goes red when the [parallel condition in the scheduler](https://github.com/temporalio/temporal/blob/44a6fe777e1dc950dd278cb1b7f6ffc8b3ba4328/service/worker/scheduler/workflow.go#L655-L660) is commented out, so it seemed the appropriate place to also test `getFutureActionTimes` lines up with the fix in the run loop. - Verified both updated tests only pass when new version (`AccurateFutureActionTimes`) is set; verified no other tests break (`make test-unit`) ## Potential risks - We break a use case that relied on speculative future action times/a constant number of action times returned ## Documentation N/A ## Is hotfix candidate? No
ychebotarev
pushed a commit
that referenced
this pull request
Jun 25, 2024
… update time, respect RemainingActions counter (#6122) ## What changed/why? In #6028/#5381, our scheduler's run loop accounted for update requests that occur between nominal and actual/jittered times. The run loop will correctly skip over action times who are scheduled prior to the schedule's update time (such as when a schedule is recalculated to a new spec). This PR updates `getFutureActionTimes` (used as part of the scheduler's describe and list operations) to account for action times preceding the update time, as well as to properly reflect the number of remaining actions in a schedule. ## How did you test it? - Updated unit tests for both limited actions (`TestLimitedActions`) as well as filtered update times (`TestUpdateNotRetroactive`). `TestUpdateNotRetroactive` is the test that goes red when the [parallel condition in the scheduler](https://github.com/temporalio/temporal/blob/44a6fe777e1dc950dd278cb1b7f6ffc8b3ba4328/service/worker/scheduler/workflow.go#L655-L660) is commented out, so it seemed the appropriate place to also test `getFutureActionTimes` lines up with the fix in the run loop. - Verified both updated tests only pass when new version (`AccurateFutureActionTimes`) is set; verified no other tests break (`make test-unit`) ## Potential risks - We break a use case that relied on speculative future action times/a constant number of action times returned ## Documentation N/A ## Is hotfix candidate? No
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changed/why?
In #6028/#5381, our scheduler's run loop accounted for update requests that occur between nominal and actual/jittered times. The run loop will correctly skip over action times who are scheduled prior to the schedule's update time (such as when a schedule is recalculated to a new spec). This PR updates
getFutureActionTimes(used as part of the scheduler's describe and list operations) to account for action times preceding the update time, as well as to properly reflect the number of remaining actions in a schedule.How did you test it?
TestLimitedActions) as well as filtered update times (TestUpdateNotRetroactive).TestUpdateNotRetroactiveis the test that goes red when the parallel condition in the scheduler is commented out, so it seemed the appropriate place to also testgetFutureActionTimeslines up with the fix in the run loop.AccurateFutureActionTimes) is set; verified no other tests break (make test-unit)Potential risks
Documentation
N/A
Is hotfix candidate?
No