-
Notifications
You must be signed in to change notification settings - Fork 340
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
Workflow Management - Initial Methods #1003
Changes from 12 commits
c2a41d9
8cd0f0e
b4498cc
94cea4d
ba0d6b8
9d2db44
3fa3012
211224d
01f257c
6688012
68e1450
f005729
3ba809f
51e4349
aaf7dcc
a704c02
e49ef1b
cff1801
dc309c3
e086a4a
bd8f2ea
770f96e
989d7c4
fba845f
f713701
55856e2
b737575
43b2035
aedece7
654b5e4
49a0c94
09e6bbf
938e349
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 | ||||
---|---|---|---|---|---|---|
|
@@ -941,6 +941,53 @@ public abstract Task<UnlockResponse> Unlock( | |||||
string lockOwner, | ||||||
CancellationToken cancellationToken = default); | ||||||
|
||||||
/// <summary> | ||||||
/// Attempt to start the given workflow with response indicating success. | ||||||
/// </summary> | ||||||
/// <param name="instanceID">Identifier of the specific run.</param> | ||||||
/// <param name="workflowComponent">The component to interface with.</param> | ||||||
/// <param name="workflowType">Name of the workflow to run.</param> | ||||||
/// <param name="workflowOptions">The list of options that are potentially needed to start a workflow.</param> | ||||||
/// <param name="input">The input input for the given workflow.</param> | ||||||
/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param> | ||||||
/// <returns>A <see cref="Task"/> containing a <see cref="WorkflowReference"/></returns> | ||||||
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")] | ||||||
public abstract Task<WorkflowReference> StartWorkflow( | ||||||
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. Normal convention in .NET is to suffix all method names that return 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. Should I we have a "Deprecation" warning to the Workflow Authoring SDK too? 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. 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. Yeah, the point of the attribute here is to make people aware they're using a new feature. If they're using a new SDK, I think that's part of the implication. Also, since we're here, I would like to have the Async in the method name (and the one below). I know we aren't consistent but we should follow standards where we can and be sad about our lack of consistency elsewhere. |
||||||
string instanceID, | ||||||
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. nit: In normal C# coding conventions, this would actually be |
||||||
string workflowComponent, | ||||||
string workflowType, | ||||||
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. Based some feedback I got from Mark, I suggest we rename workflowType to workflowName.
Suggested change
|
||||||
Object input, | ||||||
IReadOnlyDictionary<string, string> workflowOptions = default, | ||||||
CancellationToken cancellationToken = default); | ||||||
|
||||||
/// <summary> | ||||||
/// Attempt to get information about the given workflow. | ||||||
/// </summary> | ||||||
/// <param name="instanceID">Identifier of the specific run.</param> | ||||||
/// <param name="workflowComponent">The component to interface with.</param> | ||||||
/// <param name="workflowType">Name of the workflow to run.</param> | ||||||
/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param> | ||||||
/// <returns>A <see cref="Task"/> containing a <see cref="GetWorkflowResponse"/></returns> | ||||||
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")] | ||||||
public abstract Task<GetWorkflowResponse> GetWorkflow( | ||||||
string instanceID, | ||||||
string workflowComponent, | ||||||
string workflowType, | ||||||
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. It's a little awkward that I didn't notice this until now, but we don't actually need the |
||||||
CancellationToken cancellationToken = default); | ||||||
|
||||||
/// <summary> | ||||||
/// Attempt to get terminate the given workflow. | ||||||
/// </summary> | ||||||
/// <param name="instanceID">Identifier of the specific run.</param> | ||||||
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.
Suggested change
|
||||||
/// <param name="workflowComponent">The component to interface with.</param> | ||||||
/// <param name="cancellationToken">A <see cref="CancellationToken" /> that can be used to cancel the operation.</param> | ||||||
/// <returns>A <see cref="Task" /> that will complete when the operation has completed. If the wrapped value is true the operation suceeded.</returns> | ||||||
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. It's currently ambiguous as to what "operation" refers to here. For example, developers reading this might incorrectly assume that this task completes when the workflow is terminated. However, that's not actually the case. Terminate operations are normally async and aren't guaranteed to terminate the workflow right away. I suggest we say that this task completes when the terminate operation has been scheduled. |
||||||
[Obsolete("This API is currently not stable as it is in the Alpha stage. This attribute will be removed once it is stable.")] | ||||||
public abstract Task TerminateWorkflow( | ||||||
string instanceID, | ||||||
string workflowComponent, | ||||||
CancellationToken cancellationToken = default); | ||||||
|
||||||
/// <inheritdoc /> | ||||||
public void Dispose() | ||||||
{ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1374,6 +1374,118 @@ public async override Task<UnlockResponse> Unlock( | |
|
||
#endregion | ||
|
||
|
||
#region Workflow API | ||
/// <inheritdoc/> | ||
[Obsolete] | ||
public async override Task<WorkflowReference> StartWorkflow( | ||
string instanceId, | ||
string workflowComponent, | ||
string workflowType, | ||
Object input, | ||
IReadOnlyDictionary<string, string> workflowOptions = default, | ||
CancellationToken cancellationToken = default) | ||
{ | ||
ArgumentVerifier.ThrowIfNullOrEmpty(instanceId, nameof(instanceId)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(workflowComponent, nameof(workflowComponent)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(workflowType, nameof(workflowType)); | ||
ArgumentVerifier.ThrowIfNull(workflowOptions, nameof(workflowOptions)); | ||
ArgumentVerifier.ThrowIfNull(input, nameof(input)); | ||
|
||
var request = new Autogenerated.StartWorkflowRequest() | ||
{ | ||
InstanceId = instanceId, | ||
WorkflowComponent = workflowComponent, | ||
WorkflowName = workflowType, | ||
Input = (ByteString)input | ||
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. Yikes, this is not what I had in mind when I suggested allowing |
||
}; | ||
|
||
if (workflowOptions?.Count > 0) | ||
{ | ||
foreach (var item in workflowOptions) | ||
Comment on lines
+1493
to
+1495
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. Do we really need this 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 think you need it for the null check. 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. It is here to check against the possibility of workflowOptions being Null. |
||
{ | ||
request.Options[item.Key] = item.Value; | ||
} | ||
} | ||
|
||
try | ||
{ | ||
var options = CreateCallOptions(headers: null, cancellationToken); | ||
var response = await client.StartWorkflowAlpha1Async(request, options); | ||
return new WorkflowReference(response.InstanceId); | ||
|
||
} | ||
catch (RpcException ex) | ||
{ | ||
throw new DaprException("Start Workflow operation failed: the Dapr endpoint indicated a failure. See InnerException for details.", ex); | ||
} | ||
} | ||
|
||
/// <inheritdoc/> | ||
[Obsolete] | ||
public async override Task<GetWorkflowResponse> GetWorkflow( | ||
string instanceId, | ||
string workflowComponent, | ||
string workflowType, | ||
CancellationToken cancellationToken = default) | ||
{ | ||
ArgumentVerifier.ThrowIfNullOrEmpty(instanceId, nameof(instanceId)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(workflowComponent, nameof(workflowComponent)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(workflowType, nameof(workflowType)); | ||
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. Let's remove this specific argument verifier since we don't actually use the |
||
|
||
var request = new Autogenerated.GetWorkflowRequest() | ||
{ | ||
InstanceId = instanceId, | ||
WorkflowComponent = workflowComponent, | ||
WorkflowType = workflowType | ||
}; | ||
|
||
try | ||
{ | ||
var options = CreateCallOptions(headers: null, cancellationToken); | ||
var response = await client.GetWorkflowAlpha1Async(request, options); | ||
return new GetWorkflowResponse(response.InstanceId, response.StartTime, response.Metadata); | ||
} | ||
catch (RpcException ex) | ||
{ | ||
throw new DaprException("Get workflow operation failed: the Dapr endpoint indicated a failure. See InnerException for details.", ex); | ||
} | ||
|
||
} | ||
|
||
|
||
/// <inheritdoc/> | ||
[Obsolete] | ||
public async override Task TerminateWorkflow( | ||
string instanceID, | ||
string workflowComponent, | ||
CancellationToken cancellationToken = default) | ||
{ | ||
ArgumentVerifier.ThrowIfNullOrEmpty(instanceID, nameof(instanceID)); | ||
ArgumentVerifier.ThrowIfNullOrEmpty(workflowComponent, nameof(workflowComponent)); | ||
|
||
var request = new Autogenerated.TerminateWorkflowRequest() | ||
{ | ||
InstanceId = instanceID, | ||
WorkflowComponent = workflowComponent | ||
}; | ||
|
||
var options = CreateCallOptions(headers: null, cancellationToken); | ||
Autogenerated.TerminateWorkflowResponse response = new Autogenerated.TerminateWorkflowResponse(); // RRL Do we need a response? | ||
try | ||
{ | ||
response = await client.TerminateWorkflowAlpha1Async(request, options); | ||
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. We aren't returning or consuming |
||
} | ||
catch (RpcException ex) | ||
{ | ||
throw new DaprException("Terminate workflow operation failed: the Dapr endpoint indicated a failure. See InnerException for details.", ex); | ||
} | ||
|
||
} | ||
|
||
#endregion | ||
|
||
|
||
#region Dapr Sidecar Methods | ||
|
||
/// <inheritdoc/> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,56 @@ | ||||||
// ------------------------------------------------------------------------ | ||||||
// Copyright 2021 The Dapr Authors | ||||||
// 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. | ||||||
// ------------------------------------------------------------------------ | ||||||
|
||||||
using System; | ||||||
using System.Collections.Generic; | ||||||
|
||||||
namespace Dapr.Client | ||||||
{ | ||||||
/// <summary> | ||||||
/// Represents the response from invoking a binding. | ||||||
/// </summary> | ||||||
public sealed class GetWorkflowResponse | ||||||
{ | ||||||
/// <summary> | ||||||
/// Initializes a new <see cref="GetWorkflowResponse" />.` | ||||||
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. Extra character at the end:
Suggested change
|
||||||
/// </summary> | ||||||
/// <param name="instanceId">The instance ID assocated with this response.</param> | ||||||
/// <param name="startTime">The time at which the workflow started executing.</param> | ||||||
/// <param name="metadata">The response metadata.</param> | ||||||
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. nit: the whitespace needs to be fixed here too. |
||||||
public GetWorkflowResponse(string instanceId, Int64 startTime, IReadOnlyDictionary<string, string> metadata) | ||||||
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. Could the class be a record? Aside construction, we never really overwrite any of its fields, right? I guess we would lose the ability to verify those arguments if this was a record, correct? I know this can be done (https://stackoverflow.com/questions/68915019/how-to-check-if-parameters-are-null-when-using-positional-record-constructors-an) but I wonder if it would make the code any better. Again, C# is not my forté. |
||||||
{ | ||||||
ArgumentVerifier.ThrowIfNull(instanceId, nameof(instanceId)); | ||||||
ArgumentVerifier.ThrowIfNull(startTime, nameof(startTime)); | ||||||
ArgumentVerifier.ThrowIfNull(metadata, nameof(metadata)); | ||||||
|
||||||
this.InstanceId = instanceId; | ||||||
this.StartTime = startTime; | ||||||
this.Metadata = metadata; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Gets the workflow instance ID assocated with this response. | ||||||
/// </summary> | ||||||
public string InstanceId { set; get; } | ||||||
|
||||||
/// <summary> | ||||||
/// Gets the time that the workflow started. | ||||||
/// </summary> | ||||||
public Int64 StartTime { set; get; } | ||||||
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. Shouldn't this be 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. The proto definition of start_time is an int64 so StartTime in this scenario was created as an Int64 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. Yes, but it's not very friendly to present a .NET developer with an integer value when a much more friendly types are available. I think we really need to use |
||||||
|
||||||
/// <summary> | ||||||
/// Gets the response metadata from the associated workflow. This includes information such as start time and status of workflow. | ||||||
/// </summary> | ||||||
public IReadOnlyDictionary<string, string> Metadata { set; get; } | ||||||
} | ||||||
} |
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.
I feel like we've had this conversation, so forgive me. But is there a reason we went with Object instead of just a Generic? Is it just because we don't have a good means of using the generic so we get nothing from the type checking side of the world?
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.
Yeah, there's no benefit to using a generic here. We don't really care what the type of the input is. We just need to be able to serialize it to JSON.