-
Notifications
You must be signed in to change notification settings - Fork 294
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
Enable suspend-resume of orchestrators. #764
Merged
Merged
Changes from all commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
c0c48cc
Finish first draft suspend-resume feature
julio-arroyo02 77f0815
Rename to convey behavior better, update inaccurate documentation.
julio-arroyo02 4d11050
Remove unnecessary logic.
julio-arroyo02 db822e7
Avoid setting completed time when suspending or resuming an orchestra…
julio-arroyo02 1140a17
Incorporate feedback and fixes from draft PR comments
julio-arroyo02 2091db5
Refactor logic for consistency and less repetition
julio-arroyo02 b94455a
Implement tests for suspend-resume feature
julio-arroyo02 fb6fc4b
Add support for suspended orchestrations when waiting for completion
julio-arroyo02 7ab6b76
Clean up comments
julio-arroyo02 206bf94
Modularize buffer implementation for suspended orchestration messages
julio-arroyo02 775d0bb
Add new tests for suspend-resume
julio-arroyo02 a2a128f
Modify underlying data structure, change way buffer is initialized
julio-arroyo02 37639ef
Derive status from runtime, remove setter
julio-arroyo02 b847747
Add new test
julio-arroyo02 4663fcb
Refactor resuming orchestration to avoid code duplication
julio-arroyo02 bfaa1e9
Rename for consistency with other parameters
julio-arroyo02 5842118
Fix minor typo
julio-arroyo02 a2a8063
Introduce new handler to deal with events while orchestrations are su…
julio-arroyo02 202f2b4
Delete unnecessary class
julio-arroyo02 878bf2f
Address new feedback in PR
julio-arroyo02 acad657
Add minor modifications
julio-arroyo02 0745b9f
Add minor fixes
julio-arroyo02 331012f
Consolidate multiple test cases into single end-to-end test
julio-arroyo02 6924f4e
Fix EventType enum order to avoid OOProc bugs.
julio-arroyo02 44cef71
Address feedback
julio-arroyo02 d941d6f
Change patch version
julio-arroyo02 f6ba832
Fix strings.
julio-arroyo02 f280b65
Implement local orchestration's suspend-resume.
julio-arroyo02 4234747
Fix redis version.
julio-arroyo02 5e2b45f
Correct redis version.
julio-arroyo02 a766701
Correct redis version.
julio-arroyo02 c329a99
Implement new design to avoid exposing new APIs to client.
julio-arroyo02 6525c06
Refactor tests to increase reliability.
julio-arroyo02 22f9391
Fix minor version since we introduce new feature.
julio-arroyo02 8332463
Refactor logic of marker events.
julio-arroyo02 59b1cea
Put back code that should not have been removed.
julio-arroyo02 2a5e8da
Fix bug related to completed events.
julio-arroyo02 ec2f7ed
Fix AzureStorage version.
julio-arroyo02 e098959
Merge branch 'main' into suspend-resume
Julio-Arroyo 374fbf3
Fix bug: suspended orchestrations are still executable.
julio-arroyo02 f0e718d
Merge pull request #1 from Azure/main
Julio-Arroyo 4dcef91
Merge branch 'main' of https://github.com/Julio-Arroyo/durabletask in…
julio-arroyo02 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// ---------------------------------------------------------------------------------- | ||
// Copyright Microsoft Corporation | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ---------------------------------------------------------------------------------- | ||
|
||
namespace DurableTask.Core.History | ||
{ | ||
using System.Runtime.Serialization; | ||
|
||
/// <summary> | ||
/// A history event for orchestration resuming | ||
/// </summary> | ||
[DataContract] | ||
public class ExecutionResumedEvent : HistoryEvent | ||
{ | ||
/// <summary> | ||
/// Creates a new ExecutionResumedEvent with the supplied params | ||
/// </summary> | ||
/// <param name="eventId">The event id of the history event</param> | ||
/// <param name="reason">The serialized reason of the resuming event</param> | ||
public ExecutionResumedEvent(int eventId, string reason) | ||
: base(eventId) | ||
{ | ||
Reason = reason; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the event type | ||
/// </summary> | ||
public override EventType EventType => EventType.ExecutionResumed; | ||
|
||
/// <summary> | ||
/// Gets or sets the reason for the resuming event | ||
/// </summary> | ||
[DataMember] | ||
public string Reason { get; set; } | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
// ---------------------------------------------------------------------------------- | ||
// Copyright Microsoft Corporation | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// ---------------------------------------------------------------------------------- | ||
|
||
namespace DurableTask.Core.History | ||
{ | ||
using System.Runtime.Serialization; | ||
|
||
/// <summary> | ||
/// A history event for orchestration suspension. | ||
/// </summary> | ||
[DataContract] | ||
public class ExecutionSuspendedEvent : HistoryEvent | ||
{ | ||
/// <summary> | ||
/// Creates a new ExecutionSuspendedEvent with the supplied params | ||
/// </summary> | ||
/// <param name="eventId">The event id of the history event</param> | ||
/// <param name="reason">The serialized input of the suspension event</param> | ||
public ExecutionSuspendedEvent(int eventId, string reason) | ||
: base(eventId) | ||
{ | ||
Reason = reason; | ||
} | ||
|
||
/// <summary> | ||
/// Gets the event type | ||
/// </summary> | ||
public override EventType EventType => EventType.ExecutionSuspended; | ||
|
||
/// <summary> | ||
/// Gets or sets the serialized input for the the suspension event | ||
/// </summary> | ||
[DataMember] | ||
public string Reason { get; set; } | ||
} | ||
} |
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
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
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
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
Oops, something went wrong.
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.
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.
My understanding is that the latest design doesn't require any changes to the backend, so I'm not expecting this change to be necessary (in fact, I worry about potential problems it could cause). Is this a leftover from a previous design?
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.
Oh, great point. Thanks for bringing it up.
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.
@cgillum Actually, I want to argue that we do need this change:
runtimeState.OrchestrationStatus != OrchestrationStatus.Suspended
must be included. (@davidmrdavid this is the bug we were unable to explain earlier today).isExecutableInstance
has one single reference. It is used byAzureStorageOrchestrationService.cs
to check whether the batch of messages dequeued should be discarded or abandoned. If we do not include this extra check, there are many scenarios where this feature won't work. Consider the following two examples:runtimeState.OrchestrationStatus != OrchestrationStatus.Suspended
,isExecutableInstance
will return false. Which means that theRaiseEvent
will be discarded and will not make it to the history.TerminateEvent
message will be also discarded. So the terminate operation wouldn't work on suspended orchestrations.@cgillum I know you mentioned you worry about potential problems this change could introduce. Could you elaborate on that?
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.
@Julio-Arroyo Ah, I see. I was reading the logic backwards and thought we'd discard the message if it was in a suspended state. You're right - we need the suspended check here to avoid unnecessarily discarding work items for suspended instances.