-
Notifications
You must be signed in to change notification settings - Fork 31
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
Implement support for suspend/resume #195
Conversation
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.
Adding a few comments with additional context.
@@ -630,7 +630,6 @@ BEGIN | |||
E.[InstanceID] = I.[InstanceID] | |||
WHERE | |||
I.TaskHub = @TaskHub AND | |||
I.[RuntimeStatus] NOT IN ('Suspended') AND |
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.
This was added long before we actually implemented suspend/resume in DurableTask.Core, and was based on the assumption that we wouldn't even load suspended orchestrations. In the final design, however, we decided that we needed to continue loading suspended orchestrations, so this line needed to be removed.
|
||
throw; | ||
} | ||
await Task.Delay(TimeSpan.FromSeconds(1), combinedCts.Token); |
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.
The above code was deleted to ensure that we only throw OperationCanceledException
(or TaskCanceledException
) if this times out. Otherwise, sometimes we'd throw TimeoutException
and sometimes we'd throw OperationCanceledException
, depending on which line of code was running when the cancellation token was signaled.
{ | ||
AdjustTimeout(ref timeout); | ||
if (!doNotAdjustTimeout) |
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.
Needed to add this, otherwise we can't write test code that waits for timeouts (like I do in my new test) when a debugger is attached.
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.
LGTM!
Fixes #181
The MSSQL backend was missing support for suspend/resume. It also made some bad assumptions about how it would eventually behave when introduced, so some existing logic needed to be removed.
I also bundled a couple additional minor fixes into this PR: