Refactor TaskEntity to TaskEntity<TState> #175
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,11 +53,16 @@ public abstract void StartOrchestration( | |
| TaskName name, object? input = null, StartOrchestrationOptions? options = null); | ||
|
|
||
| /// <summary> | ||
| /// Gets the current state for the entity this context is for. This will return <c>null</c> if no state is present, | ||
| /// regardless if <paramref name="type"/> is a value-type or not. | ||
| /// </summary> | ||
| /// <param name="type">The type to retrieve the state as.</param> | ||
| /// <returns>The entity state.</returns> | ||
| public abstract object? GetState(Type type); | ||
|
There was a problem hiding this comment. For clarity, it may be nice to document how There was a problem hiding this comment. Another option would be simply to support only nullable types for entities, then we don't have this strange corner case. I think I prefer this more restrictive solution.... there is something very strange about setting an integer entity to zero and then see it disappear from storage. It is clever, but this will invariably confuse a lot of people. There was a problem hiding this comment. I thought the An alternative is There was a problem hiding this comment.
The problem with that is that you get strange behaviors. For example, for There was a problem hiding this comment. Per our offline discussion, setting We will also add a built in There was a problem hiding this comment. just to clarify: reset is not identical to delete as far as the user is concerned because a reset entity will still be returned by subsequent queries, while a deleted entity is not. Our users definitely care about this distinction. we had endless discussions about this and many issues, e.g. Azure/azure-functions-durable-extension#931 (comment). |
||
|
|
||
| /// <summary> | ||
| /// Sets the entity state. Setting of <c>null</c> will clear entity state. | ||
|
There was a problem hiding this comment. I suggest 'delete' instead of 'clear' since it will delete records from storage, not just reset the entity to its initial state. This makes a difference, for example when querying entities. |
||
| /// </summary> | ||
| /// <param name="state">The state to set.</param> | ||
| public abstract void SetState(object? state); | ||
| } | ||
|
There was a problem hiding this comment. I would suggest to add an overload method for deleting state: It's redundant (and we can say that in the comment) but when I write or read code I would much rather use |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Reflection; | ||
|
|
||
| namespace Microsoft.DurableTask.Entities; | ||
|
|
||
| /// <summary> | ||
| /// Helpers for task entities. | ||
| /// </summary> | ||
| static class TaskEntityHelpers | ||
| { | ||
| /// <summary> | ||
| /// Unwraps a dispatched result for a <see cref="TaskEntityOperation"/> into a <see cref="ValueTask{Object}"/>. | ||
| /// </summary> | ||
| /// <param name="context">The entity context.</param> | ||
| /// <param name="state">Delegate to resolve new state for the entity.</param> | ||
| /// <param name="result">The result of the operation.</param> | ||
| /// <param name="resultType">The declared type of the result (may be different that actual type).</param> | ||
| /// <returns>A value task which holds the result of the operation and sets state before it completes.</returns> | ||
| public static ValueTask<object?> UnwrapAsync( | ||
| TaskEntityContext context, Func<object?> state, object? result, Type resultType) | ||
| { | ||
| // NOTE: Func<object?> is used for state so that we can lazily resolve it AFTER the operation has ran. | ||
| Check.NotNull(context); | ||
| Check.NotNull(resultType); | ||
|
|
||
| if (typeof(Task).IsAssignableFrom(resultType)) | ||
| { | ||
| // Task or Task<T> | ||
| // We assume a declared Task return type is never null. | ||
| return new(UnwrapTask(context, state, (Task)result!, resultType)); | ||
| } | ||
|
|
||
| if (resultType == typeof(ValueTask)) | ||
| { | ||
| // ValueTask | ||
| // We assume a declared ValueTask return type is never null. | ||
| return UnwrapValueTask(context, state, (ValueTask)result!); | ||
| } | ||
|
|
||
| if (resultType.IsGenericType && resultType.GetGenericTypeDefinition() == typeof(ValueTask<>)) | ||
| { | ||
| // ValueTask<T> | ||
| // No inheritance, have to do purely via reflection. | ||
| return UnwrapValueTaskOfT(context, state, result!, resultType); | ||
| } | ||
|
|
||
| context.SetState(state()); | ||
| return new(result); | ||
| } | ||
|
|
||
| static async Task<object?> UnwrapTask(TaskEntityContext context, Func<object?> state, Task task, Type declared) | ||
| { | ||
| await task; | ||
| context.SetState(state()); | ||
| if (declared.IsGenericType && declared.GetGenericTypeDefinition() == typeof(Task<>)) | ||
| { | ||
| return declared.GetProperty("Result", BindingFlags.Public | BindingFlags.Instance).GetValue(task); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| static ValueTask<object?> UnwrapValueTask(TaskEntityContext context, Func<object?> state, ValueTask t) | ||
| { | ||
| async Task<object?> Await(ValueTask t) | ||
| { | ||
| await t; | ||
| context.SetState(state()); | ||
| return null; | ||
| } | ||
|
|
||
| if (t.IsCompletedSuccessfully) | ||
| { | ||
| context.SetState(state()); | ||
| return default; | ||
| } | ||
|
|
||
| return new(Await(t)); | ||
| } | ||
|
|
||
| static ValueTask<object?> UnwrapValueTaskOfT( | ||
| TaskEntityContext context, Func<object?> state, object result, Type type) | ||
| { | ||
| // Result and type here must be some form of ValueTask<T>. | ||
| // TODO: can this amount of reflection be avoided? | ||
| if ((bool)type.GetProperty("IsCompletedSuccessfully").GetValue(result)) | ||
| { | ||
| context.SetState(state()); | ||
| return new(type.GetProperty("Result").GetValue(result)); | ||
| } | ||
| else | ||
| { | ||
| Task t = (Task)type.GetMethod("AsTask", BindingFlags.Instance | BindingFlags.Public).Invoke(result, null); | ||
| Type taskType = typeof(Task<>).MakeGenericType(type.GetGenericArguments()[0]); | ||
| return new(UnwrapTask(context, state, t, taskType)); | ||
| } | ||
| } | ||
| } |
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.
@sebastianburckhardt want to draw your attention to this. I set the default to
falsebecause I feel like that is safer. Don't want users accidentally dispatching to state when they don't realize this property & behavior exists.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.
Setting this to false by default makes sense for the way this interface is currently defined. However, I think there is still a gap here.
I think the dilemma here is about whether we are trying to provide (a) a flexible interface that is constructed logically and can support many different types of
TStateand (b) an interface that aggressively minimizes boilerplate for the most common and simplest use where the user wants to just write the entity just as a simple single class with methods to define the operations, i.e. they only want to look at the classTStateand have the existence ofTaskEntity<TState>be an implementation detail that gets hidden in some minimal boilerplate or perhaps even hidden completely at some point.As currently designed,
TaskEntity<TState>matches the requirements for (a), but not (b). I realize it may not be possible to satisfy both requirements at the same time..But we really also need something for (b) because it just seems to matter the most in my experience - it is how we want to write all our samples and benchmarks. I remember that for DF we designed multiple APIs but when I was writing applications it turned out I only ever wanted the single-class one because the code looks so much nicer that way. Having the state indirection is a pretty big hassle IMO, and forcing our users to do that is a step backwards (allowing them to do it is fine).
So, I still think we need to somehow find a way to make that simple single-class experience with minimal boilerplate possible. Maybe use a derived class
ClassBasedEntity<TClass>and then constrain all the choices to keep things simple: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 current priorities I would be o.k. with delaying the design and implementation of this "easier to use" single-class version and focus on the more verbose and flexible TaskEntity for now.
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.
Per our offline discussion, current experience is sufficient for now. All of these scenarios are possible, but might not be a 1 to 1 with entities in-proc. Which is OK, we can expect some changes during migration. We will revisit this later based on user demand.