Skip to content
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

Bring entity logic into DurableTask.Core (first milestone) #887

Merged
merged 13 commits into from
Aug 29, 2023

Conversation

sebastianburckhardt
Copy link
Collaborator

As discussed in #862 (comment), this PR is a reduced form of the original PR #862.

It contains the implementaton of entity mechanics, compatible with existing DF SDK, but not the user-facing entity SDK for DTFx.

…ut without a user-facing entity SDK for DTFx
@cgillum
Copy link
Member

cgillum commented Apr 14, 2023

Looks like some tests are failing. A couple of them are in DurableTask.Core.Tests.CustomExceptionsTests and appear to require some additional code changes (or test changes).

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finished my first round of review of this code, covering all files. There was a lot of skimming involved, however, since it's still pretty huge. I focused mostly on style, structure, public surface area, and other misc. things like logging.

src/DurableTask.Core/Entities/ClientEntityContext.cs Outdated Show resolved Hide resolved
src/DurableTask.Core/Entities/ClientEntityContext.cs Outdated Show resolved Hide resolved
src/DurableTask.Core/Entities/EntityBackendInformation.cs Outdated Show resolved Hide resolved
src/DurableTask.Core/Entities/IEntityExecutor.cs Outdated Show resolved Hide resolved
#nullable enable
namespace DurableTask.Core.Entities.OperationFormat
{
using Newtonsoft.Json;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for us to use System.Runtime.Serialization instead of Newtonsoft.Json in these files? Ideally DT.Core standardizes on just the former for defining data contracts. This PR generally seems to use a mix of both and I just want to confirm whether it's intentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I don't have a very good idea about what serializers are used and why in all the different environments, I was just following the identical approach as what is already being used in OrchestratorAction.cs, since that appears to function satisfactorily.

IMO those two should always be kept consistent. If we want to revise this, we should revise both. And probably not in this PR since it is already so big.

src/DurableTask.Core/TaskHubWorker.cs Outdated Show resolved Hide resolved
src/DurableTask.Core/TaskHubWorker.cs Show resolved Hide resolved
src/DurableTask.Core/TaskHubWorker.cs Show resolved Hide resolved
src/DurableTask.Core/TaskOrchestrationContext.cs Outdated Show resolved Hide resolved
{
// TODO : mark an orchestration as faulted if there is data corruption
this.logHelper.DroppingOrchestrationWorkItem(workItem, "Received work-item for an invalid orchestration");
TraceHelper.TraceSession(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a copy/paste of the TaskOrchestrationDispatcher code? I wouldn't expect that we'd want to use these old TraceHelper classes. I'd rather we not carry them forward in new code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a copy/paste

Yes.

I'd rather we not carry them forward in new code

I will remove them all from the TaskEntityDispatcher.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, can't remove TraceHelper.TraceExceptionInstance since it throws an exception. Will keep that for consistency with the other two dispatchers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at TaskOrchestrationDispatcher, I can see that there are many calls to TraceHelper (the old way of doing tracing) that do not have a corresponding call to LogHelper (the new way of doing tracing). I am not sure if this was

  1. a conscious choice (as in: we really don't want to do so much logging anymore) or
  2. a question of priorities (as in: I don't have time to port all this tracing code right now).

If the reason is 1. then we are done here. I suspect the reason may have been 2., however.

Given that the code paths are almost exactly the same for TaskOrchestrationDispatcher and TaskEntityDispatcher, it really does not make any sense to write LogHelper methods just for TaskEntityDispatcher. The right design would be to call the same methods from both dispatchers (along with an indication of which dispatcher is calling them).

The question remains whether it makes sense to tack the "port tracehelper to loghelper" work item onto this PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that there are many calls to TraceHelper (the old way of doing tracing) that do not have a corresponding call to LogHelper (the new way of doing tracing).

I recall when doing this work that I intentionally didn't add logs for certain TraceHelper events because I thought they were too noisy or redundant - so (1) is most likely in the cases you looked at. That said, it's not a big deal if we want to just keep the existing ones in the code - however, we don't generally consider them when doing Kusto queries for production issues.

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more small things

// See the License for the specific language governing permissions and
// limitations under the License.
// ----------------------------------------------------------------------------------
namespace DurableTask.Core.Entities
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add #nullable enable on these new classes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to almost all new classes. There are a few exceptions; some classes were ported from previously existing code and would require a lot of annotations or maybe changes, it did not seem worth the effort (and risk) to extensively modify them.

using System.Collections.Generic;

/// <summary>
/// Orchestrator action for creating sub-orchestrations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see that from the class name, but not from this summary description. Should it instead say, "Entity action for creating sub-orchestrations"?

@@ -85,6 +98,7 @@ public TaskHubWorker(IOrchestrationService orchestrationService, ILoggerFactory
/// <param name="orchestrationService">Reference the orchestration service implementation</param>
/// <param name="orchestrationObjectManager">NameVersionObjectManager for Orchestrations</param>
/// <param name="activityObjectManager">NameVersionObjectManager for Activities</param>
/// <remarks></remarks>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this empty comment block?

/// <returns></returns>
public TaskHubWorker AddTaskEntities(params Type[] taskEntityTypes)
{
if (this.SupportsEntities)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (this.SupportsEntities)
if (!this.SupportsEntities)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, not sure how this escaped testing. Must have not run the tests on latest changes yet.

isExtendedSession = this.concurrentSessionLock.Acquire();
this.concurrentSessionLock.Release();
workItem.IsExtendedSession = isExtendedSession;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove all references to CorrelationTraceClient. This is a preview feature that we're planning to remove anyways so I'd rather not add new references to it.

Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got around making a full pass (some skimming was necessary). Some questions and comments

Comment on lines 68 to 82
/// Initializes a new instance of the <see cref="TaskOrchestrationExecutor"/> class.
/// This overload is needed only to avoid breaking changes because this is a public constructor.
/// </summary>
/// <param name="orchestrationRuntimeState"></param>
/// <param name="taskOrchestration"></param>
/// <param name="eventBehaviourForContinueAsNew"></param>
/// <param name="errorPropagationMode"></param>
public TaskOrchestrationExecutor(
OrchestrationRuntimeState orchestrationRuntimeState,
TaskOrchestration taskOrchestration,
BehaviorOnContinueAsNew eventBehaviourForContinueAsNew,
ErrorPropagationMode errorPropagationMode = ErrorPropagationMode.SerializeExceptions)
: this(orchestrationRuntimeState, taskOrchestration, eventBehaviourForContinueAsNew, new EntityBackendProperties(), errorPropagationMode)
{
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm this corresponds to the existing constructor, which we're re-creating as an overload now that we've added a new parameter (entityBackendProperties) to the constructor we used to have, right?

Also, I noticed the new EntityBackendProperties parameter in the old constructor is nullable (i.e has a ?). Given this, why do we need to pass a newly constructed EntityBackendProperties object to it instead of null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a leftover from an earlier version that was always passing a non-null object. I agree that this should be passing null, which means anyone who is using the legacy constructor runs without using the new entity dispatcher.

renewCancellationTokenSource.Token);
}

try
{
// Assumes that: if the batch contains a new "ExecutionStarted" event, it is the first message in the batch.
if (!this.ReconcileMessagesWithState(workItem))
if (!ReconcileMessagesWithState(workItem, "TaskOrchestrationDispatcher", logHelper))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to do this?

Suggested change
if (!ReconcileMessagesWithState(workItem, "TaskOrchestrationDispatcher", logHelper))
if (!ReconcileMessagesWithState(workItem, nameof(TaskOrchestrationDispatcher), logHelper))

Comment on lines 122 to 127
// we call the entity interface to make sure we are receiving only true orchestrations here
return this.entityOrchestrationService.LockNextOrchestrationWorkItemAsync(receiveTimeout, cancellationToken);
}
else
{
return this.orchestrationService.LockNextTaskOrchestrationWorkItemAsync(receiveTimeout, cancellationToken);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "to make sure we are receiving only true orchestrations here", "here" actually refers to the else-case, right? I found this a bit confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment.

/// <summary>
/// Information about backend entity support, or null if the configured backend does not support entities.
/// </summary>
internal EntityBackendProperties EntityBackendProperties { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this might be null, shouldn't it be EntityBackendProperties?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class does not have #nullable enable.
(It is a pre-existing class so I am not changing that)

@@ -85,6 +98,7 @@ public TaskHubWorker(IOrchestrationService orchestrationService, ILoggerFactory
/// <param name="orchestrationService">Reference the orchestration service implementation</param>
/// <param name="orchestrationObjectManager">NameVersionObjectManager for Orchestrations</param>
/// <param name="activityObjectManager">NameVersionObjectManager for Activities</param>
/// <remarks></remarks>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this if there's no remarks?

Suggested change
/// <remarks></remarks>

processCount++;
}

// Fetches beyond the first require getting an extended session lock, used to prevent starvation.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand this comment. Can we rephrase this? What is "the first require"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic and wording is slightly weird, but this was just copy-pasted from existing code. I am not actually sure why this would be called without new messages, then there really is no "first fetch".


DateTime timestamp = now;

if (SendHorizon + reorderWindow + MinIntervalBetweenCollections < now)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you've walked me through this in the past, but I still get a bit confused by the MinIntervalBetweenCollections term in this if statement. The expression makes perfect sense to me if it were just SendHorizon + reorderWindow < now, and I get quickly confused when I try to figure out how the interval bit affects this calculation.

Can we add a clarifying comment in here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a longer comment.

// deliver any messages that were held in the receive buffers
// but are now past the reorder window

List<string> emptyReceiveBuffers = new List<string>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - I think a better name would be "buffersToRemove".
The name emptyReceiveBuffers makes me think they were always empty, but in fact they become cleared in this method.

Comment on lines 134 to 137
if (kvp.Value.Last < ReceiveHorizon)
{
kvp.Value.Last = DateTime.MinValue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to add clarity, assuming this is correct:

Suggested change
if (kvp.Value.Last < ReceiveHorizon)
{
kvp.Value.Last = DateTime.MinValue;
}
// Check if buffered messages are ready to be delivered
if (kvp.Value.Last < ReceiveHorizon)
{
// flag as ready to be delivered
kvp.Value.Last = DateTime.MinValue;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to write comments about this. The algorithm is complicated and I don't fully remember myself. We would need a TLA+ model to really document how this thing works.

Comment on lines 144 to 148
if (kvp.Value.Last == DateTime.MinValue
&& (kvp.Value.Buffered == null || kvp.Value.Buffered.Count == 0))
{
emptyReceiveBuffers.Add(kvp.Key);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - to add more clarity, assuming my understanding is correct:

Suggested change
if (kvp.Value.Last == DateTime.MinValue
&& (kvp.Value.Buffered == null || kvp.Value.Buffered.Count == 0))
{
emptyReceiveBuffers.Add(kvp.Key);
}
// determine if buffer is empty, so it can be removed
if (kvp.Value.Last == DateTime.MinValue
&& (kvp.Value.Buffered == null || kvp.Value.Buffered.Count == 0))
{
emptyReceiveBuffers.Add(kvp.Key);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comment.

Copy link
Collaborator

@jviau jviau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed you had some logic about processing entities together or separately. This is great and I think that will help the dotnet isolated implementation.

For dotnet isolated it will help for us to process them together, so that we can dispatch an entity using the existing gRPC protocol to the worker. Then on the worker side after deserialization we can differentiate between entity and orchestration and invoke them differently. For this to work, it will be helpful for us to have access to the following logic:

  1. Something that can differentiate between orchestration and entity (fine if this is copy & pasted code, but well defined rules for us to follow would be great)
  2. A TaskEntityExecutor, which takes in the same arguments as TaskOrchestrationExecutor, but a TaskEntity instead of TaskOrchestration. We will handle constructing the TaskEntity object and invoking it.

@@ -22,32 +21,61 @@ namespace DurableTask.AzureStorage
/// See https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function.
/// Tested with production data and random guids. The result was good distribution.
/// </remarks>
static class Fnv1aHashHelper
public static class Fnv1aHashHelper
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to add something like this to the public API space. Can we make a copy? Or link it in to each csproj explicitly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a copy for DurableTask.AzureStorage

/// <summary>
/// Utility functions for clients that interact with entities, either by sending events or by accessing the entity state directly in storage
/// </summary>
public static class ClientEntityHelpers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are customers expected to use this directly? Does this need to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used by the SDKs that implement the entity API. Those are not the customers directly, all such SDKs are meant to be written by our team. So this class, though technically public, should be considered internal from a support perspective. We are not officially supporting scenarios where anyone outside of our team is using this class (they are not supposed to).

For example, this is used by the implementation of AzureStorageDurabilityProvider in webjobs-extensions-durabletask. And it would likely be what you want to use when implementing the client in the isolated SDK.

/// <param name="input">The serialized input for the operation.</param>
/// <param name="scheduledTimeUtc">The time to schedule this signal, or null if not a scheduled signal</param>
/// <returns>The event to send.</returns>
public static EventToSend EmitOperationSignal(OrchestrationInstance targetInstance, Guid requestId, string operationName, string input, (DateTime original, DateTime capped)? scheduledTimeUtc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: upper-case value tuple names

Suggested change
public static EventToSend EmitOperationSignal(OrchestrationInstance targetInstance, Guid requestId, string operationName, string input, (DateTime original, DateTime capped)? scheduledTimeUtc)
public static EventToSend EmitOperationSignal(OrchestrationInstance targetInstance, Guid requestId, string operationName, string input, (DateTime Original, DateTime Capped)? scheduledTimeUtc)

Copy link
Collaborator

@jviau jviau May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, use DateTimeOffset in favor of DateTime - it is a better structure overall (it is more robust, includes explicit timezone value directly into it).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the tuple names. Not changing DateTimeOffset for now, I think it is better to stay consistent with the rest of the code which is using DateTime. All of our DateTime are always Utc anyway.

/// <summary>
/// Computes a cap on the scheduled time of an entity signal, based on the maximum signal delay time
/// </summary>
/// <param name="nowUtc"></param>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fill in comments please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

/// The name for this class of entities.
/// </summary>
[DataMember(Name = "name", IsRequired = true)]
public readonly string Name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use properties not fields for anything beyond private:

Suggested change
public readonly string Name;
public string Name { get; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, trying this out for Name and Key fields.

/// A unique identifier for an entity, consisting of entity name and entity key.
/// </summary>
[DataContract]
public readonly struct EntityId : IEquatable<EntityId>, IComparable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it is worth adding what we need to access record structs in this project? It would simplify a lot of your logic, as it comes with equality, hash code, all that:

public record struct EntityId(string Name, string Key)
{
    // override ToString, add FromString, and other methods as necessary.
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it is already done I see little motivation to replace it with a different way to do the same thing. Will keep in mind for future code though.

case OperationActionType.StartNewOrchestration:
return new StartNewOrchestrationOperationAction();
default:
throw new NotSupportedException("Unrecognized action type.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like it would be a very high volume log. Would it offer enough value to warrant it?

{
if (this.IsInsideCriticalSection)
{
foreach(var e in this.availableLocks!)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach(var e in this.availableLocks!)
foreach (var e in this.availableLocks!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

await this.dispatcher.StopAsync(forced);
}


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove newline

Suggested change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

readonly ErrorPropagationMode errorPropagationMode;
readonly TaskOrchestrationDispatcher.NonBlockingCountdownLock concurrentSessionLock;


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@sebastianburckhardt
Copy link
Collaborator Author

For dotnet isolated it will help for us to process them together, so that we can dispatch an entity using the existing gRPC protocol to the worker. Then on the worker side after deserialization we can differentiate between entity and orchestration and invoke them differently.

I think it would be cleaner to update the gRPC protocol so it supports entity work items directly. Since we are revising this anyway, I see little benefit to treating entity work items and orchestration work items the same way. We did this in the previous implementation and it just makes things more complicated than necessary, it is a lesson we have learnt. Orchestrations and entities execute very differently. I would like to represent entity work items as batches of operations, there is no history and no replay involved.

@sebastianburckhardt sebastianburckhardt changed the base branch from main to feature/core-entities August 28, 2023 22:55
@sebastianburckhardt sebastianburckhardt merged commit adcaf61 into feature/core-entities Aug 29, 2023
@sebastianburckhardt sebastianburckhardt deleted the draft-core-entities-minimum branch August 29, 2023 16:23
sebastianburckhardt added a commit that referenced this pull request Oct 17, 2023
* Bring entity logic into DurableTask.Core (first milestone) (#887)

* implementaton of entity mechanics, compatible with existing DF SDK, but without a user-facing entity SDK for DTFx

* address PR feedback.

* fix usings and namespaces

* address PR feedback

* address PR feedback (remove NameObjectManager), fix breaking change in TaskHubWorker, fix some comments

* address PR feedback (fix CustomExceptionsTest, remove public property)

* add #nullable enable to most new classes

* address PR feedback

* try to fix compiler errors

* add a configuration setting that disables separate dispatch by default

* address PR feedback

* address PR feedback

* fix semantic merge conflict.

* Revise entity state and status format and access (#955)

* update scheduler state and entity status format and helpers

* fix mess-up caused by merge conflict

* Revise entity message format and external access (#956)

* revise how event messages are represented and used

* fix merge anomaly.

* make current critical section id publicly visible (#958)

* remove orchestration tags from entity action (#952)

* Rename OperationBatchRequest and OperationBatchResponse (#953)

* rename OperationBatch to EntityBatch

* fix accidentally commited change from another PR

* Revise how entity batches are executed and handle failures (#954)

* revise task entity definition

* commit change that had been accidentally committed to a different PR.

* Apply suggestions from code review

Co-authored-by: David Justo <david.justo.1996@gmail.com>

* Apply suggestions from code review

Co-authored-by: Jacob Viau <javia@microsoft.com>

---------

Co-authored-by: David Justo <david.justo.1996@gmail.com>
Co-authored-by: Jacob Viau <javia@microsoft.com>

* revise operation result encoding and add more comments. (#965)

* revise entity backend properties and implement entity backend queries (#957)

* revise entity backend properties and implement entity backend queries.

* Minor revisions to querries and properties, and improved comments.

* fix validation of which LockNext methods are being called.

* improve comments

* fix useage of IEntityOrchestrationService.

* revise how to exclude entity results from queries.

* address PR feedback

* Update versions for ADO feed (#973)

* Add no-warn for NU5104 (#974)

* revise propagation path for entity parameters (#971)

* fix propagation path for entity parameters that need to reach the orchestration executor.

* address PR feedback.

* Revise entity queries (#981)

* rename includeDeletedEntities to includeStatelessEntities and add comment explaining the meaning

* add backlogQueueSize and lockedBy to entity metadata

* fix bugs in tracking store implementation (#979)

* add scheduled start time parameter to the start-new-orchestration operation action. (#980)

* Revise serialization of entitymessages (#972)

* revise how entity messages are serialized when sent by orchestrators.

* address PR feedback (use RawInput)

* Rename includeStateless to includeTransient in entity queries (#985)

* rename includeStateless to includeTransient

* rename variable also

* Rev to entities-preview.2 (#986)

* fix null reference exception when running on older backends (#989)

* Prepare for public preview (#994)

---------

Co-authored-by: David Justo <david.justo.1996@gmail.com>
Co-authored-by: Jacob Viau <javia@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants