Skip to content
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

Merged
merged 5 commits into from
Nov 3, 2023
Merged

Implement support for suspend/resume #195

merged 5 commits into from
Nov 3, 2023

Conversation

cgillum
Copy link
Member

@cgillum cgillum commented Nov 2, 2023

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:

  • Fixed inconsistent exception throwing in SqlOrchestrationService.WaitForOrchestrationAsync (realized this was a problem while writing my suspend/resume test)
  • Fixed minor issue with default DateTime handling (force it to always be UTC) - this is an issue with certain kinds of serializers, like protobuf, which reject DateTime values that aren't explicitly UTC (found this doing some unrelated exploratory prototyping).

@cgillum cgillum self-assigned this Nov 2, 2023
Copy link
Member Author

@cgillum cgillum left a 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
Copy link
Member Author

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);
Copy link
Member Author

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)
Copy link
Member Author

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.

Copy link
Contributor

@bachuv bachuv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cgillum cgillum merged commit ceddafa into main Nov 3, 2023
2 checks passed
@cgillum cgillum deleted the suspend-resume branch November 3, 2023 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SqlUtils doesn't support suspending
2 participants