-
Notifications
You must be signed in to change notification settings - Fork 321
Log warning on multiple execution completed events and allow termination to override previous execution completed events #949
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
Changes from all commits
2c3fa68
4deb515
3b27f08
f45b4cf
4241a01
d146047
6f865c4
5a068aa
c23d3cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,6 +18,7 @@ namespace DurableTask.Core | |||||||||||||||||||
| using System.Diagnostics; | ||||||||||||||||||||
| using DurableTask.Core.Common; | ||||||||||||||||||||
| using DurableTask.Core.History; | ||||||||||||||||||||
| using DurableTask.Core.Logging; | ||||||||||||||||||||
| using DurableTask.Core.Tracing; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
|
|
@@ -184,6 +185,14 @@ public OrchestrationStatus OrchestrationStatus | |||||||||||||||||||
| this.Events.Count == 1 && this.Events[0].EventType == EventType.OrchestratorStarted || | ||||||||||||||||||||
| this.ExecutionStartedEvent != null; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Gets or sets a LogHelper instance that can be used to log messages. | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
| /// <remarks> | ||||||||||||||||||||
| /// Ideally, this would be set in the constructor but that would require a larger refactoring. | ||||||||||||||||||||
| /// </remarks> | ||||||||||||||||||||
| internal LogHelper? LogHelper { get; set; } = null; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Adds a new history event to the Events list and NewEvents list | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
|
|
@@ -260,14 +269,33 @@ void SetMarkerEvents(HistoryEvent historyEvent) | |||||||||||||||||||
| } | ||||||||||||||||||||
| else if (historyEvent is ExecutionCompletedEvent completedEvent) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (ExecutionCompletedEvent != null) | ||||||||||||||||||||
| if (ExecutionCompletedEvent == null) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| throw new InvalidOperationException( | ||||||||||||||||||||
| "Multiple ExecutionCompletedEvent found, potential corruption in state storage"); | ||||||||||||||||||||
| ExecutionCompletedEvent = completedEvent; | ||||||||||||||||||||
| orchestrationStatus = completedEvent.OrchestrationStatus; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // It's not generally expected to receive multiple execution completed events for a given orchestrator, but it's possible under certain race conditions. | ||||||||||||||||||||
| // For example: when an orchestrator is signaled to terminate at the same time as it attempts to continue-as-new. | ||||||||||||||||||||
| var log = $"Received new {completedEvent.GetType().Name} event despite the orchestration being already in the {orchestrationStatus} state."; | ||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not a fan of this log change. You're saying we received an event but you're not saying what we're doing with it. The previous log made it clear that we're ignoring the new event.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cgillum: I think there might be some confusion here. The log grows to include whether we decided to ignore the event later on. We don't always ignore the new event, as seen in the snippet below: durabletask/src/DurableTask.Core/OrchestrationRuntimeState.cs Lines 287 to 295 in 5a068aa
So it's a log message that's created in two parts. First we capture that we're in a situation where we have to make a decision of which ExecutionCompletedEvent to preserve, and we include which one we decide to preserve (the new one or the previous one).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. In that case it's fine, but I think you need to add a space between the period in the first half of the log and the start of the next sentence.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed in latest commit: c23d3cc |
||||||||||||||||||||
|
|
||||||||||||||||||||
| if (orchestrationStatus == OrchestrationStatus.ContinuedAsNew && completedEvent.OrchestrationStatus == OrchestrationStatus.Terminated) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // If the orchestration planned to continue-as-new but termination is requested, we transition to the terminated state. | ||||||||||||||||||||
| // This is because termination should be considered to be forceful. | ||||||||||||||||||||
| log += " Discarding previous 'ExecutionCompletedEvent' as termination is forceful."; | ||||||||||||||||||||
| ExecutionCompletedEvent = completedEvent; | ||||||||||||||||||||
| orchestrationStatus = completedEvent.OrchestrationStatus; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // otherwise, we ignore the new event. | ||||||||||||||||||||
| log += " Discarding new 'ExecutionCompletedEvent'."; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| LogHelper?.OrchestrationDebugTrace(this.OrchestrationInstance?.InstanceId ?? "", this.OrchestrationInstance?.ExecutionId ?? "", log); | ||||||||||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did not find a pre-existing loghelper method that fit this case. |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ExecutionCompletedEvent = completedEvent; | ||||||||||||||||||||
| orchestrationStatus = completedEvent.OrchestrationStatus; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else if (historyEvent is ExecutionSuspendedEvent) | ||||||||||||||||||||
| { | ||||||||||||||||||||
|
|
||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
It seemed to me that we were missing a general "debug trace" for logs like this one, so I decided to create one. It did not seem appropriate to create a very specific new category just for this log, so instead I created a more general one. I think this is a good idea considering that, as we recently learned with the entity logs, adding new EventIDs is expensive / can take a long time to deploy in Antares, so I wanted to make sure we have access to more general logging methods.