Provide entity support for direct grpc connections to DTS by sebastianburckhardt · Pull Request #369 · microsoft/durabletask-dotnet

sebastianburckhardt · 2025-01-27T23:46:48Z

implements the necessary protocol conversions and configurations for providing entity support when connecting a DTS backend directly ("portable SDK").

sebastianburckhardt · 2025-01-29T23:44:23Z

I am marking this as ready for review now that I have some tests passing.

Given that iterations are slow, I want to run a lot more tests before merging though.

YunchuWang · 2025-01-30T19:50:42Z

LGTM

jviau · 2025-02-03T21:07:19Z

src/Client/AzureManaged/DurableTaskSchedulerClientExtensions.cs

            .Configure(additionalConfig ?? (_ => { }))
            .ValidateDataAnnotations();

+        builder.Services.AddOptions<DurableTaskClientOptions>(builder.Name)


I think there is a builder.Configure(optionsCallback) overload you can use

yes, much better.

jviau · 2025-02-03T21:13:41Z

src/Worker/Grpc/GrpcDurableTaskWorkerOptions.cs

+    /// automatically converted back and forth between the old DT Core representation (JSON-encoded external events)
+    /// and the new protobuf representation (explicit history events), which is used by the DTS scheduler backend.
+    /// </summary>
+    public bool ConvertOrchestrationEntityEvents { get; set; }


This value and InsertEntityUnlocksOnCompletion look like they are more implementation details and not a public API. I would prefer we figure out a way to make these non-public if possible. If this must be public due to different projects, then we should find a way to hide this in an .Internal namespace and add comments declaring this not meant for public consumption.

Here is an example: https://github.com/microsoft/durabletask-dotnet/blob/main/src/Abstractions/Internal/IOrchestrationSubmitter.cs

One idea would be to make these properties internal (maybe even in a sub-class to keep them together). Then add an extension method ConfigureForAzureManaged(this GrpcDurableTaskWorkerOptions options) under Microsoft.DurableTask.Worker.Grpc.Internal namespace with the comments. Then AzureManaged can call that.

Would it be possible to use an "internals visible to" property so that only Worker.AzureManaged can access these options?

Yes, you can do that for now. I did it in the Abstraction package:

https://github.com/microsoft/durabletask-dotnet/blob/main/src/Abstractions/Abstractions.csproj#L22-L23

Please just add a comment saying these are used by AzureManaged to treat them as public API breaking change rules

But I have since changed my stance on how much I like that (I don't recommend it anymore). The reason is this can still cause issues down the road. If customers use Worker.AzureManager/1.n which uses these cross-assembly internal properties, but also use Worker.Grpc/1.n+1 which has since removed these properties in a refactor, the app will throw method missing exceptions.

BUT given we already use this pattern in the code base, I think the comment will be good enough. Maybe in a v2.x we can rethink and remove this approach.

I tried the "internals visible" approach and it worked but it is also causing some warnings (because there are conflicts between doubly defined classes, those come from files that are shared between the projects).

So I will instead do what you suggested before.

I have modified the code so it now uses a private class InternalOptions, and offers a public extension method ConfigureForAzureManaged(this GrpcDurableTaskWorkerOptions options) like you suggested.

cgillum · 2025-02-06T01:18:15Z

src/Worker/Grpc/InternalOptionsExtensions.cs

+    /// Configure the worker to use the default settings for connecting to the Azure Managed Durable Task service.
+    /// </summary>
+    /// <param name="options">The gRPC worker options.</param>
+    public static void ConfigureForAzureManaged(this GrpcDurableTaskWorkerOptions options)


Can this be moved to the AzureManaged project? Code in the Grpc project is supposed to be backend-agnostic.

This is based on a suggestion of mine. The properties being added in InternalOptions are specialized for AzureManaged scenario, but they need to be in the project. To avoid having these options that customer's are not expected to ever configure, we make the values themselves internal and only expose a single public point to configure them.

With that said, we can name this method more backend-agnostic, but it does need to remain in this project so it can access the internal properties. Additionally, we should include a disclaimer in the XML doc explaining this is considered an internal API surface and not subject to breaking change guarantees.

I kept the name (we will confuse only ourselves if we invent more names) but there is now an XML doc that states that this is an internal API.

cgillum · 2025-02-06T01:27:42Z

src/Shared/Grpc/EntityConversions.cs

+
+    // The point of this class is to reverse a Newtonsoft serialization that happened in prior DT code.
+    // To do this reliably we use the same Newtonsoft.
+    // This is not introducing a new dependency, and should be eliminated once the original dependency is eliminated.


Just to be clear, if we remove the DT.Core dependency from this SDK, will that also allow us to remove this dependency on Newtonsoft.Json?

Yes. The point of this class is to "undo Newtonsoft serialization" that was unfortunately already done inside DT.Core. So the Newtonsoft dependency goes away when either (a) Newtonsoft dependency is removed from DT.Core, or (b) DT.Core dependency is removed.

cgillum · 2025-02-06T01:31:08Z

@sebastianburckhardt you'll need to rebase this PR on the latest from main. We no longer use submodules for updating the protobuf definitions. See this change: #370.

jviau · 2025-02-06T16:40:00Z

src/Worker/Grpc/InternalOptionsExtensions.cs

+using System.Collections.Generic;
+using System.Text;
+
+namespace Microsoft.DurableTask.Worker.Grpc.Internal;


Please move to an Internal folder.

jviau · 2025-02-06T16:40:26Z

src/Worker/Grpc/InternalOptions.cs

+/// Options for controlling backend-specific features. These are not exposed directly, but can be accessed via
+/// the extension methods in <see cref="Internal.InternalOptionsExtensions"/>.
+/// </summary>
+class InternalOptions


Consider doing one of the following:

Make this a nested class in GrpcDurableTaskWorkerOptions.

Or, rename to InternalWorkerOptions and move to Internal folder (along with the extension class for it).

I am making this a nested class.

src/Worker/Grpc/Worker.Grpc.csproj

jviau · 2025-02-06T16:42:12Z

src/Worker/Grpc/GrpcDurableTaskWorkerOptions.cs

-    /// unlock events into the history when an orchestration terminates while holding an entity lock.
-    /// </summary>
-    public bool InsertEntityUnlocksOnCompletion { get; set; }
+    internal InternalOptions InternalOptions { get; set; } = new();


I recommend removing the setter (unless it is needed)

Suggested change

internal InternalOptions InternalOptions { get; set; } = new();

internal InternalOptions InternalOptions { get; } = new();

src/Worker/Grpc/GrpcDurableTaskWorkerOptions.cs

jviau · 2025-02-10T17:10:40Z

eng/proto

I thought this was removed

Will merge second-to-latest commit in main which introduced the vendor pattern.

I rebased on latest main now so this is no longer there.

jviau · 2025-02-10T17:11:40Z

src/Worker/Grpc/InternalOptionsExtensions.cs

+/// <summary>
+/// Provides access to configuring internal options for the gRPC worker.
+/// </summary>
+public static class InternalOptionsExtensions


Please include disclaimers as seen on this type: https://github.com/microsoft/durabletask-dotnet/blob/main/src/Abstractions/Internal/IOrchestrationSubmitter.cs

I have added an XML comment on the extension method

sebastianburckhardt · 2025-02-11T22:00:50Z

eng/proto

For some reason this is still showing up. Will try to get rid of it.

sebastianburckhardt mentioned this pull request Jan 28, 2025

Add orchestrator actions for entity messages microsoft/durabletask-protobuf#31

Merged

sebastianburckhardt force-pushed the sburckha/dts-entities branch from 7f3fc0a to 8db3603 Compare January 29, 2025 23:24

sebastianburckhardt marked this pull request as ready for review January 29, 2025 23:43

YunchuWang approved these changes Jan 30, 2025

View reviewed changes

jviau reviewed Feb 3, 2025

View reviewed changes

cgillum reviewed Feb 6, 2025

View reviewed changes

jviau reviewed Feb 10, 2025

View reviewed changes

sebastianburckhardt commented Feb 11, 2025

View reviewed changes

eng/proto Outdated

Copy link

Member Author

sebastianburckhardt Feb 11, 2025

Choose a reason for hiding this comment

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

For some reason this is still showing up. Will try to get rid of it.

implement entity support for DTS (squashed commit and rebased)

0490d86

sebastianburckhardt force-pushed the sburckha/dts-entities branch from 3d04a45 to 0490d86 Compare February 11, 2025 22:13

sebastianburckhardt merged commit 52afe9c into main Feb 13, 2025
4 checks passed

sebastianburckhardt deleted the sburckha/dts-entities branch February 13, 2025 00:04

	internal InternalOptions InternalOptions { get; set; } = new();
	internal InternalOptions InternalOptions { get; } = new();

Conversation

sebastianburckhardt commented Jan 27, 2025

Uh oh!

sebastianburckhardt commented Jan 29, 2025

Uh oh!

YunchuWang commented Jan 30, 2025

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 Feb 5, 2025 • 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!

sebastianburckhardt Feb 5, 2025 • 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!

cgillum commented Feb 6, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jviau Feb 5, 2025 •

edited

Loading

sebastianburckhardt Feb 5, 2025 •

edited

Loading