-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Feature/cancel dispatched workflows #5136
Conversation
/// <summary> | ||
/// Splits an enumerable into batches of a specified size. | ||
/// </summary> | ||
public static IEnumerable<IEnumerable<T>> ToBatches<T>(this IEnumerable<T> enumerable, int pageLimit) |
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.
Given the name of this method, it sounds like you could use the built-in Chunk extension.
@@ -1,3 +1,4 @@ | |||
using System.Diagnostics.CodeAnalysis; |
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.
Unnecessary import.
} | ||
} | ||
catch (ServiceBusException e) when(e.Reason == ServiceBusFailureReason.MessagingEntityNotFound) | ||
{ |
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.
Let's add a logging statement here (perhaps of level "warning").
@@ -28,6 +28,11 @@ public class WorkflowState | |||
/// </summary> | |||
public int DefinitionVersion { get; set; } | |||
|
|||
/// <summary> | |||
/// The ID of the workflow which dispatched this workflow. |
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 description does not seem to reflect the property's purpose.
@@ -22,6 +22,11 @@ public class WorkflowInstance : Entity | |||
/// The version of the workflow definition. | |||
/// </summary> | |||
public int Version { get; set; } | |||
|
|||
/// <summary> | |||
/// The ID of the workflow which dispatched this workflow. |
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 description does not seem to reflect the property's purpose.
foreach (var workflowInstanceIdBatch in workflowInstanceIdBatches) | ||
{ | ||
// Find child instances for the current workflow instance ID and cancel them. | ||
// Do not check on status as their |
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 comment looks sounds like its incomplete.
private async Task<int> CancelChildWorkflowInstances(IEnumerable<string> workflowInstanceIds, CancellationToken cancellationToken) | ||
{ | ||
var tasks = new List<Task<int>>(); | ||
var workflowInstanceIdBatches = workflowInstanceIds.ToBatches(50); |
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.
workflowInstanceIds.Chunk(50)
should work too.
Ensures workflows that are dispatched gets cancelled.
There are 2 known issues:
Both issues will be picked up through issues #5031 and #4835 respectively
Closes #4177