Add versioning support in durabletask-dotnet(phase1) #295

skyao · 2024-04-22T08:26:21Z

Code updates:

TaskName

Add a new constructor that sets the version expected by the client.

The existing code has a version field and associated implementation, but no constructor to set the version is provided. Adding this constructor makes it possible to do so in client-side code:
```
string instanceId = await client.ScheduleNewOrchestrationInstanceAsync(
  new TaskName(nameof(HelloOrchestration), "1.2.0"));
```

TaskOrchestrationContext

Add new abstract method to get instance version.

So that in the orchestration code, we get the instance version by this context:

    public static async Task<List<string>> RunOrchestrator(
        [OrchestrationTrigger] TaskOrchestrationContext context)
    {
			string instanceVersion = context.InstanceVersion;
      ......
    }

TaskOrchestrationContextWrapper

Implemented new added abstract method to get instance version.

This method will get the value of instance version from OrchestrationContext.Version.

This PR is depended on another PR in durable repo.

Azure/durabletask#1071

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

skyao · 2024-04-22T08:27:52Z

This PR is depended on the PR of Azure/durabletask#1071 to build.

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

src/Abstractions/TaskName.cs

src/Abstractions/TaskOrchestrationContext.cs

… breaking change. Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

…et into versioning-phase1

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

jviau

I am a bit concerned about the usability of this API. Both behavior we had before this PR, but also behavior afterwards.

Right now my biggest concern is we have a semantic of using : as the separator for name & version when going to string. We never restricted characters in the task name before, but we have a need for it now (restricting :) which is a breaking change. Wonder how we can work through this.

src/Abstractions/TaskName.cs

jviau · 2024-05-21T16:08:43Z

src/Abstractions/TaskName.cs

-        {
-            return this.Name + ":" + this.Version;
-        }
+      return this.Name + ":" + this.Version;


We should consider adding FromString semantics. But I wonder if that will be a breaking change? We have not restricted what characters can be used in a task name before this.

This change is going to introduce a confusing behavior:

TaskName name1 = new("MyTask", "2"); // Name = "MyTask", Version = "2' TaskName name2 = name1.ToString(); // implicit operator convers. Name = "MyTask:2", Version = ""

Should the implicit operator be updated to parse out the version?

I'm not super concerned about the breaking change since 99+% of .NET users should be relying on the function name, which already can't have special characters. However, we can take a look at Kusto data to confirm.

Yes the implicit operator would be changed to call FromString.

src/Abstractions/TaskName.cs

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

skyao · 2024-07-10T07:48:21Z

In this PR I just want to pass version field correctly so that it can used to support versioning feature. But I don't know if I should update the related method like string() / Equals() / GetHashCode() because before this PR these methods ignored version field.

I'm afraid that after I updated these methods (string() / Equals() / GetHashCode()) to include version field but not only name field, there are some broken changes to exist code.

@jviau @cgillum please help to make a decision that should I update these methods to include version field.

src/Abstractions/TaskName.cs

jviau · 2024-07-10T16:19:57Z

src/Abstractions/TaskName.cs

+  /// <param name="version">The version of the task.</param>
+  public TaskName(string name, string version)
+  {
+    if (name is null)


This is a good opportunity for pattern matching. Can replace this if/else statements with:

(this.Name, this.Version) = (name, version) switch { (null, null) => (null!, null!), // Force the default struct when null is passed in. (null, string _) => throw new ArgumentException("name must not be null when version is non-null"), (string _, null) => (name, string.Empty), (string _, string _) => (name, version), };

Also while we are at it, can replace the other ctor with:

(this.Name, this.Version) = name switch { null => (null!, null!), // Force the default struct when null is passed in. string _ => (name, string.Empty), }

jviau · 2024-07-10T16:31:24Z

In this PR I just want to pass version field correctly so that it can used to support versioning feature. But I don't know if I should update the related method like string() / Equals() / GetHashCode() because before this PR these methods ignored version field.

I'm afraid that after I updated these methods (string() / Equals() / GetHashCode()) to include version field but not only name field, there are some broken changes to exist code.

@jviau @cgillum please help to make a decision that should I update these methods to include version field.

The from string conversion is the only breaking change I see. We need to parse the version out of the string, otherwise round-tripping between string and back will give different results. But if we choose a delimiter a customer already uses, it can break their orchestrations. But I am not sure what other choice we have if we want to introduce Version.

@cgillum what do you think? Do we introduce a behavior breaking change?

cgillum

Added some comments. One thing that's odd to me is that we don't roundtrip the version information when converting between TaskName and string. I left a comment about this.

The breaking change part might be something we can confirm with telemetry. I can help with this.

src/Abstractions/TaskName.cs

cgillum · 2024-07-10T16:49:24Z

src/Abstractions/TaskName.cs

-        {
-            return this.Name + ":" + this.Version;
-        }
+      return this.Name + ":" + this.Version;


Should the implicit operator be updated to parse out the version?

I'm not super concerned about the breaking change since 99+% of .NET users should be relying on the function name, which already can't have special characters. However, we can take a look at Kusto data to confirm.

src/Abstractions/TaskName.cs

jviau · 2024-07-10T17:12:54Z

One thing that's odd to me is that we don't roundtrip the version information when converting between TaskName and string.

Yes, we definitely need to add this. @skyao can you also add public static TaskName FromString(string taskName) and have the string conversion operator call this. It should parse out name and version based on the convention we settle on.

jviau · 2024-07-10T17:58:10Z

src/Abstractions/TaskName.cs

-        {
-            return this.Name + ":" + this.Version;
-        }
+      return this.Name + ":" + this.Version;


We should have a discussion on what separator to use. I like @, as that is what we have used for distributed tracing to indicate the version - and ":" was used to separate the task type: {task_type}:{name}@{version}

However, we already use @ to separate entity name and key - will that be an issue? Or is that a good thing to use @ as a separator in both for consistency.

I think using @ could break entity ID parsing. Looking at this code, I can see that entity IDs take the form of {name}@{key} and the code that parses this looks for the first instance of @ to separate the name from the key. If we inject a version into the name, we make it {name}@{version}@{key}, and the key for entities will be parsed as {version}@{key} instead of {key}.

@cgillum yeah I am trying to think through how that behaves. One thing is that it is a separate type for entities. We have a few options:

use @ for separator. Entity instance IDs can then look like @{entity}@{version}@{key} (or we can do key first, then version). RISK: keys could already contain @. This could break those instances.

@sebastianburckhardt suggested we could prefix with double @@ to indicate this entity has a version field. So @myEntity@some@key -> no version field. @@myEntity@version@some@key -> version field present.

use a different character. @{entity}?{version}@{key} (or key first). RISK: this character could already be used by customers in name or key

Alternatively, we could opt to not support versioning of entities just yet

Want to clarify that I am not opposed to a different separator char for version. We haven't officially GA'd distributed tracing v2, so we could update that spec with the new separator. But what one exactly?

Some that I have ruled out myself in the past:

/ - avoided this so it wasn't confused with HTTP routes in distributed tracing

% - often used for encoding special chars, so wanted to avoid that

* - wild card, wanted to avoid that.

! - used in netherite for partition targeting. We are looking to also support that in other backends.

: - used in distributed tracing to separate the task-type. ie: orchestration:name@version or activity:name@version

Can we merge this PR first and then open a new issue to trace and update the separator after we have a final decision?

Because today is my last day, I'm afraid that I can't continue to update the code in this PR because of permissions.

@cgillum @jviau please have a look at this PR and let's make the final decision.

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

…tor to call FromString() method Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

skyao · 2024-07-18T07:18:39Z

One thing that's odd to me is that we don't roundtrip the version information when converting between TaskName and string.

Yes, we definitely need to add this. @skyao can you also add public static TaskName FromString(string taskName) and have the string conversion operator call this. It should parse out name and version based on the convention we settle on.

Done, code updated. Please have a look.

skyao added 3 commits April 22, 2024 16:08

Add a new constructor that sets the version expected by the client

380c622

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

Add new abstract method to get instance version

1976de6

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

Implemented new added abstract method to get instance version

4e1f8cb

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

change to get instance version from this.innerContext.Version

f6162ff

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

skyao mentioned this pull request Apr 23, 2024

Add versioning support(phase1) Azure/azure-functions-durable-extension#2799

Open

15 tasks

jviau requested changes Apr 25, 2024

View reviewed changes

src/Abstractions/TaskName.cs Show resolved Hide resolved

src/Abstractions/TaskName.cs Outdated Show resolved Hide resolved

src/Abstractions/TaskOrchestrationContext.cs Outdated Show resolved Hide resolved

skyao added 3 commits April 26, 2024 15:54

Merge branch 'main' into versioning-phase1

d564c79

change InstanceVersion to virtual and return null be default to avoid…

d9298d0

… breaking change. Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

Merge branch 'versioning-phase1' of github.com:skyao/durabletask-dotn…

60c1341

…et into versioning-phase1

skyao changed the title ~~Add versioning support in durablttask-dotnet(phase1)~~ Add versioning support in durabletask-dotnet(phase1) Apr 29, 2024

skyao added 4 commits April 29, 2024 16:19

Merge branch 'main' into versioning-phase1

fc44a4a

Merge branch 'main' into versioning-phase1

5b1b2b2

Merge branch 'main' into versioning-phase1

cb86ecd

update constructor to handle null correctly

83bc227

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

jviau requested changes May 21, 2024

View reviewed changes

skyao added 3 commits July 5, 2024 11:12

Merge branch 'main' into versioning-phase1

f1619b1

update Equals/GetHashCode method to include version field

0045ced

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

use value.ToString() to include version field

2172e96

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

jviau reviewed Jul 10, 2024

View reviewed changes

src/Abstractions/TaskName.cs Outdated Show resolved Hide resolved

jviau reviewed Jul 10, 2024

View reviewed changes

src/Abstractions/TaskName.cs Show resolved Hide resolved

jviau reviewed Jul 10, 2024

View reviewed changes

cgillum reviewed Jul 10, 2024

View reviewed changes

src/Abstractions/TaskName.cs Outdated Show resolved Hide resolved

jviau reviewed Jul 10, 2024

View reviewed changes

skyao added 2 commits July 16, 2024 15:41

rollback file formator to use 4 space indentation

5c3ad58

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

use Microsoft.Bcl.HashCode to do hash

7912d46

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

skyao added 3 commits July 16, 2024 15:47

fix typo

d015d3e

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

udpate documents for version

87b15e1

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

add static FromString() method and update the string conversion opera…

6ecfde1

…tor to call FromString() method Signed-off-by: Sky Ao <aoxiaojian@gmail.com>

Merge branch 'main' into versioning-phase1

220fb2d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add versioning support in durabletask-dotnet(phase1) #295

Add versioning support in durabletask-dotnet(phase1) #295

skyao commented Apr 22, 2024 •

edited

Loading

skyao commented Apr 22, 2024

jviau left a comment

jviau May 21, 2024

cgillum Jul 10, 2024

jviau Jul 10, 2024

skyao commented Jul 10, 2024

jviau Jul 10, 2024

jviau commented Jul 10, 2024

cgillum left a comment

cgillum Jul 10, 2024

jviau commented Jul 10, 2024

jviau Jul 10, 2024

cgillum Jul 16, 2024

jviau Jul 18, 2024

jviau Jul 18, 2024

skyao Jul 19, 2024

skyao Aug 6, 2024

skyao commented Jul 18, 2024

Add versioning support in durabletask-dotnet(phase1) #295

Are you sure you want to change the base?

Add versioning support in durabletask-dotnet(phase1) #295

Conversation

skyao commented Apr 22, 2024 • edited Loading

skyao commented Apr 22, 2024

jviau left a 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

skyao commented Jul 10, 2024

Choose a reason for hiding this comment

jviau commented Jul 10, 2024

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviau commented Jul 10, 2024

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

skyao commented Jul 18, 2024

skyao commented Apr 22, 2024 •

edited

Loading