Provide entity support for direct grpc connections to DTS #369

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").

…on history events, as used by DTS, and enable entities by default when using DTS backend.

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

… of failure details

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

src/Client/AzureManaged/DurableTaskSchedulerClientExtensions.cs

@@ -85,6 +85,12 @@ static void ConfigureSchedulerOptions(
            .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.

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.

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

Co-authored-by: Jacob Viau <[email protected]>

sebastianburckhardt mentioned this pull request Jan 28, 2025

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

Merged

implement conversion to new protobuf-based entity-related orchestrati…

8db3603

…on history events, as used by DTS, and enable entities by default when using DTS backend.

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

fix configuration of orchestration entity parameters, and propagation…

1690405

… of failure details

jviau reviewed Feb 3, 2025

View reviewed changes

address PR feedback.

e5473d6

cgillum reviewed Feb 6, 2025

View reviewed changes

jviau reviewed Feb 10, 2025

View reviewed changes

sebastianburckhardt and others added 2 commits February 10, 2025 11:46

incorporate suggestion from PR feedback

1e4af3d

Co-authored-by: Jacob Viau <[email protected]>

incorporate suggestion from PR feedback

45f07a7

Co-authored-by: Jacob Viau <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide entity support for direct grpc connections to DTS #369

Provide entity support for direct grpc connections to DTS #369

sebastianburckhardt commented Jan 27, 2025

sebastianburckhardt commented Jan 29, 2025

YunchuWang commented Jan 30, 2025

jviau Feb 3, 2025

sebastianburckhardt Feb 6, 2025

jviau Feb 3, 2025

sebastianburckhardt Feb 5, 2025

jviau Feb 5, 2025 •

edited

Loading

sebastianburckhardt Feb 5, 2025 •

edited

Loading

sebastianburckhardt Feb 6, 2025

cgillum Feb 6, 2025

jviau Feb 6, 2025

cgillum Feb 6, 2025

sebastianburckhardt Feb 10, 2025

cgillum commented Feb 6, 2025

jviau Feb 6, 2025

jviau Feb 6, 2025

sebastianburckhardt Feb 10, 2025

jviau Feb 6, 2025

jviau Feb 10, 2025

sebastianburckhardt Feb 10, 2025

jviau Feb 10, 2025

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

Provide entity support for direct grpc connections to DTS #369

Are you sure you want to change the base?

Provide entity support for direct grpc connections to DTS #369

Conversation

sebastianburckhardt commented Jan 27, 2025

sebastianburckhardt commented Jan 29, 2025

YunchuWang commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviau Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

sebastianburckhardt Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgillum commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviau Feb 5, 2025 •

edited

Loading

sebastianburckhardt Feb 5, 2025 •

edited

Loading