revise entity backend properties and implement entity backend queries by sebastianburckhardt · Pull Request #957 · Azure/durabletask

sebastianburckhardt · 2023-09-06T23:29:19Z

This PR does

simplify how features are supported via IEntityOrchestrationService.
implements direct support for entity queries in the AzureStorageOrchestrationService.

davidmrdavid · 2023-09-18T22:16:43Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

               MaxConcurrentTaskEntityWorkItems = this.settings.MaxConcurrentTaskEntityWorkItems,
               SupportsImplicitEntityDeletion = false, // not supported by this backend
               MaximumSignalDelayTime = TimeSpan.FromDays(6),
+               UseSeparateQueueForEntityWorkItems = this.settings.UseSeparateQueueForEntityWorkItems,


What is this "use separate queue" feature?

This means the backend maintains two separate queues for orchestration work items and entity work items. Among other things, this makes it possible to specify separate concurrency limits for the two.

davidmrdavid

A few questions from a quick pass

davidmrdavid · 2023-09-18T22:17:20Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+           => new EntityTrackingStoreQueries(
+                this.messageManager,
+                (this.trackingStore as AzureTableTrackingStore)
+                    ?? throw new NotSupportedException("entity queries not supported for custom tracking stores"),


nit:

Suggested change

?? throw new NotSupportedException("entity queries not supported for custom tracking stores"),

?? throw new NotSupportedException("entity queries are not supported for custom tracking stores"),

Also, why separate tracking stores not supported?

I thought I needed to access some internal methods of AzureTableTrackingStore. However, now that I look at it again, that seems not actually necessary. Will change it so all tracking stores are supported.

davidmrdavid · 2023-09-18T22:18:34Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+            if (this.settings.UseSeparateQueueForEntityWorkItems)
+            {
+                throw new InvalidOperationException("backend was configured for separate orchestration/entity processing, must use specialized methods to get work items");
+            }


This error message is not particularly customer actionable as far as I can tell. What can the customer do to deal with this error?

If the customer sees this we messed up. So the message is really for us. I can modify the text to make this clearer.

davidmrdavid · 2023-09-18T22:19:24Z

src/DurableTask.Core/Entities/EntityBackendProperties.cs

 namespace DurableTask.Core.Entities
 {
    using System;
+    using System.Threading;


do we really need this here?

It seems needed by one of the references in the XML comments.

davidmrdavid · 2023-09-18T22:20:22Z

src/DurableTask.Core/Entities/IEntityOrchestrationService.cs

+        /// <returns>An object containing properties of the entity backend, or null if the backend does not natively support entities.</returns>
+        EntityBackendProperties? EntityBackendProperties { get; }


Is this about supporting entities in general, or entity queries in particular?

It's about direct support for Durable.Task.Core entities in the backend. Relevant mostly for allowing separate dispatch. For out-of-proc v2 SDKs, this is a requirement for supporting entities. For the old SDKs, they can continue to work without this support (but we may allow them to take advantage).

I have updated the comment to explain a bit more.

davidmrdavid · 2023-09-18T22:25:15Z

src/DurableTask.AzureStorage/EntityTrackingStoreQueries.cs

+            while ( // continue query right away if the page is completely empty, but never in excess of 100ms
+                continuationToken != null
+                && entityResult.Count == 0
+                && stopwatch.ElapsedMilliseconds <= 100);


have we always had this 100ms limit?

yes, this was already there (but it was in the DF extension since that's where we implemented this query).

jviau · 2023-09-20T21:36:00Z

src/DurableTask.Core/Entities/EntityBackendQueries.cs

+        /// <summary>
+        /// Metadata about an entity, as returned by queries.
+        /// </summary>
+        public struct EntityMetadata


Recommend moving all these out of a nested class. I only use public nested class types very sparingly.

I prefer the nesting since it is much easier to keep everything that belongs together in one place.

jviau · 2023-09-20T21:41:30Z

Do you know why tests are failing? Is that going to be addressed afterwards?

sebastianburckhardt · 2023-09-21T20:49:56Z

I am going to take a look at the failing tests after I merge this (because it is difficult to maintain different branches for all the PRs in my local build).

* 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>

revise entity backend properties and implement entity backend queries.

c3905c8

sebastianburckhardt marked this pull request as draft September 6, 2023 23:29

sebastianburckhardt added dt.azurestorage DurableTask.AzureStorage dt.core DurableTask.Core core-entities Support for entities labels Sep 6, 2023

sebastianburckhardt added the P1 label Sep 7, 2023

sebastianburckhardt added 5 commits September 8, 2023 13:38

Minor revisions to querries and properties, and improved comments.

add83fa

fix validation of which LockNext methods are being called.

04da90a

improve comments

dc32f6f

fix useage of IEntityOrchestrationService.

a0672eb

revise how to exclude entity results from queries.

b113ae0

sebastianburckhardt marked this pull request as ready for review September 14, 2023 17:07

sebastianburckhardt mentioned this pull request Sep 14, 2023

simplify how entities are excluded from instance queries Azure/azure-functions-durable-extension#2586

Merged

davidmrdavid reviewed Sep 18, 2023

View reviewed changes

address PR feedback

c43b8d9

jviau approved these changes Sep 20, 2023

View reviewed changes

sebastianburckhardt merged commit e729bb6 into feature/core-entities Sep 21, 2023

	?? throw new NotSupportedException("entity queries not supported for custom tracking stores"),
	?? throw new NotSupportedException("entity queries are not supported for custom tracking stores"),

		/// <returns>An object containing properties of the entity backend, or null if the backend does not natively support entities.</returns>
		EntityBackendProperties? EntityBackendProperties { get; }

Conversation

sebastianburckhardt commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidmrdavid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianburckhardt Sep 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau commented Sep 20, 2023

Uh oh!

sebastianburckhardt commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sebastianburckhardt commented Sep 6, 2023 •

edited

Loading

sebastianburckhardt Sep 19, 2023 •

edited

Loading