-
Notifications
You must be signed in to change notification settings - Fork 168
Updated tracking / transitions of the task's version state. #9127
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
base: master
Are you sure you want to change the base?
Conversation
b9c6f9e to
01d362e
Compare
| // suppose to run on the instance we just failed to create. | ||
| // If this doesn't work, we'll eventually retry. Hence, correctness | ||
| // does not hinge on this transaction being successful. | ||
| await db.tasks.restorePreviousVersionsState( |
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'm pretty sure we shouldn't have a method for this, should be inline logic.
There is never a reason to call restorePreviousVersionsState from anywhere else is there?
Also you cannot look at restorePreviousVersionsState and not understand the context within which it belongs.
if we want it as a method, then it should be a helper method in this file, like updatePackageStateWithPendingVersions (which IMO could also be inline); but I do think updatePackageStateWithPendingVersions is a bit more specific in what it does and worth documenting separately. Also just to keep the code short.
Why can't we simply store previousScheduled as a local variable in scope of scheduleInstance?
01d362e to
95fe45c
Compare
Co-authored-by: Jonas Finnemann Jensen <jopsen@gmail.com>
Co-authored-by: Jonas Finnemann Jensen <jopsen@gmail.com>
Co-authored-by: Jonas Finnemann Jensen <jopsen@gmail.com>
docsandpanaflags after scheduling (otherwise there is a time window where a new schedule has been started but not completed, where a previous run's results are not available).previousScheduledfield, but we could skip it and just useinitialTimestampwhen restoring the previous state.updatePackageStateWithPendingVersionsandrestorePreviousVersionsStatesince they no longer need to keep all the previous version state information.