-
Notifications
You must be signed in to change notification settings - Fork 374
Propagated history changes #1833
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
35a7532
3cd096a
9dac517
c447a0c
e59f1d0
eafe005
aafaafc
c6ce7af
333b6c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,65 +28,62 @@ namespace Dapr.Workflow; | |
| /// <param name="activities">Activities resolved from this entry, in execution order.</param> | ||
| /// <param name="childWorkflows">Child workflows resolved from this entry, in execution order.</param> | ||
| /// <remarks> | ||
| /// One <see cref="PropagatedHistoryEntry"/> exists per ancestor workflow in a | ||
| /// One <see cref="PropagatedHistoryEvent"/> exists per ancestor workflow in a | ||
| /// <see cref="PropagatedHistory"/>. Use <see cref="TryGetLastActivityByName"/> and | ||
| /// <see cref="TryGetLastChildWorkflowByName"/> to look up specific items in this entry; | ||
|
Contributor
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. Maybe I missed this in our DMs, we do support a top level In go, off of
Contributor
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'm not super worried about having a bunch of variations of the filters. I expose all the events in a property, so if there's some special filter a developer wishes to apply, they can trivially add it themselves. Also, yes, .NET has filters on the list of all propagated history events to return a single |
||
| /// <see cref="TryGetLastWorkflowByName"/> to look up specific items in this entry; | ||
| /// the plural <c>Get*ByName</c> variants return every occurrence in execution order. | ||
| /// </remarks> | ||
| public sealed class PropagatedHistoryEntry( | ||
| public sealed class PropagatedHistoryEvent( | ||
| string instanceId, | ||
| string appId, | ||
| string name, | ||
| IReadOnlyList<PropagatedHistoryActivityResult> activities, | ||
| IReadOnlyList<PropagatedHistoryChildWorkflowResult> childWorkflows) | ||
| IReadOnlyList<PropagatedHistoryWorkflowResult> childWorkflows) | ||
| { | ||
| private readonly IReadOnlyList<PropagatedHistoryActivityResult> _activities = | ||
| activities ?? throw new ArgumentNullException(nameof(activities)); | ||
| private readonly IReadOnlyList<PropagatedHistoryChildWorkflowResult> _childWorkflows = | ||
| childWorkflows ?? throw new ArgumentNullException(nameof(childWorkflows)); | ||
|
|
||
| /// <summary>The instance ID of the ancestor workflow this entry describes.</summary> | ||
| /// <summary>The instance ID of the workflow this history event describes.</summary> | ||
| public string InstanceId { get; } = instanceId ?? throw new ArgumentNullException(nameof(instanceId)); | ||
|
|
||
| /// <summary>The Dapr App ID that ran this ancestor workflow.</summary> | ||
| /// <summary>The Dapr App ID that ran this workflow.</summary> | ||
| public string AppId { get; } = appId ?? throw new ArgumentNullException(nameof(appId)); | ||
|
|
||
| /// <summary>The name of this ancestor workflow.</summary> | ||
| /// <summary>The name of this workflow.</summary> | ||
| public string Name { get; } = name ?? throw new ArgumentNullException(nameof(name)); | ||
|
|
||
| /// <summary>All activities executed in this entry, in execution order.</summary> | ||
| public IReadOnlyList<PropagatedHistoryActivityResult> Activities => _activities; | ||
| /// <summary>All activities executed in this history event, in execution order.</summary> | ||
| public IReadOnlyList<PropagatedHistoryActivityResult> Activities { get; } = | ||
| activities ?? throw new ArgumentNullException(nameof(activities)); | ||
|
|
||
| /// <summary>All child workflows started in this entry, in execution order.</summary> | ||
| public IReadOnlyList<PropagatedHistoryChildWorkflowResult> ChildWorkflows => _childWorkflows; | ||
| /// <summary>All workflows started in this history event, in execution order.</summary> | ||
| public IReadOnlyList<PropagatedHistoryWorkflowResult> Workflows { get; } = | ||
| childWorkflows ?? throw new ArgumentNullException(nameof(childWorkflows)); | ||
|
|
||
| /// <summary> | ||
| /// Returns every activity in this entry whose scheduled name matches, in execution order. | ||
| /// Returns every activity in this history event whose scheduled name matches, in execution order. | ||
| /// </summary> | ||
| /// <param name="name">The activity name to filter by.</param> | ||
| /// <returns>An empty list when no match is found.</returns> | ||
| public IReadOnlyList<PropagatedHistoryActivityResult> GetActivitiesByName(string name) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | ||
| return _activities | ||
| return Activities | ||
| .Where(a => string.Equals(a.Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tries to return the most recent activity in this entry whose name matches. | ||
| /// Tries to return the most recent activity in this history event whose name matches. | ||
| /// </summary> | ||
| /// <param name="name">The activity name to look up.</param> | ||
| /// <param name="result">When this method returns <see langword="true"/>, the last matching activity; otherwise <see langword="null"/>.</param> | ||
| /// <returns><see langword="true"/> if a matching activity was found; otherwise <see langword="false"/>.</returns> | ||
| public bool TryGetLastActivityByName(string name, [NotNullWhen(true)] out PropagatedHistoryActivityResult? result) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | ||
| for (var i = _activities.Count - 1; i >= 0; i--) | ||
| for (var i = Activities.Count - 1; i >= 0; i--) | ||
| { | ||
| if (string.Equals(_activities[i].Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| if (string.Equals(Activities[i].Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| result = _activities[i]; | ||
| result = Activities[i]; | ||
| return true; | ||
| } | ||
| } | ||
|
|
@@ -96,32 +93,32 @@ public bool TryGetLastActivityByName(string name, [NotNullWhen(true)] out Propag | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns every child workflow in this entry whose name matches, in execution order. | ||
| /// Returns every child workflow in this history event whose name matches, in execution order. | ||
| /// </summary> | ||
| /// <param name="name">The child workflow name to filter by.</param> | ||
| /// <returns>An empty list when no match is found.</returns> | ||
| public IReadOnlyList<PropagatedHistoryChildWorkflowResult> GetChildWorkflowsByName(string name) | ||
| public IReadOnlyList<PropagatedHistoryWorkflowResult> GetWorkflowsByName(string name) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | ||
| return _childWorkflows | ||
| return Workflows | ||
| .Where(c => string.Equals(c.Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tries to return the most recent child workflow in this entry whose name matches. | ||
| /// Tries to return the most recent child workflow in this history event whose name matches. | ||
| /// </summary> | ||
| /// <param name="name">The child workflow name to look up.</param> | ||
| /// <param name="result">When this method returns <see langword="true"/>, the last matching child workflow; otherwise <see langword="null"/>.</param> | ||
| /// <returns><see langword="true"/> if a matching child workflow was found; otherwise <see langword="false"/>.</returns> | ||
| public bool TryGetLastChildWorkflowByName(string name, [NotNullWhen(true)] out PropagatedHistoryChildWorkflowResult? result) | ||
| public bool TryGetLastWorkflowByName(string name, [NotNullWhen(true)] out PropagatedHistoryWorkflowResult? result) | ||
| { | ||
| ArgumentException.ThrowIfNullOrWhiteSpace(name); | ||
| for (var i = _childWorkflows.Count - 1; i >= 0; i--) | ||
| for (var i = Workflows.Count - 1; i >= 0; i--) | ||
| { | ||
| if (string.Equals(_childWorkflows[i].Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| if (string.Equals(Workflows[i].Name, name, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| result = _childWorkflows[i]; | ||
| result = Workflows[i]; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
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.
Why
Try? All the other sdks haveGetLastWorkflowEventByNamewithoutTry. Is this more of a dotnet consistency thing?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.
A
nullin a response can mean so many things. Taking a recent example from the state store - if you query a value from state and you get a null back, does that mean that the value didn't exist or that it did and it is null? ATryGetanswers this definitively.Especially as these are not asynchronous operations, I'd rather return a clear boolean indicating if getting the value(s) worked or not and include a payload with the corresponding result if it did.