Add version to TaskName #416
Conversation
halspang
force-pushed
the
halspang/taskname_version
branch
from
4cd811b to
766fa69
Compare
|
@jviau - I wasn't aware of that context, thanks for linking it. I'll go through it and see if I can determine a path forward there. In the meantime, I separated out the context change as I think this change should be isolated and that's the more important change that I need for versioning functionality in the extension. |
This commit adds an explict set for Version in TaskName. It also includes a flag that is set so other code can determine if the TaskName is intended to be versioned. Finally, it updates the Version property in the TaskOrchestrationContext as that was always set to empty instead of being able to be used. Signed-off-by: halspang <[email protected]>
Signed-off-by: halspang <[email protected]>
halspang
force-pushed
the
halspang/taskname_version
branch
from
766fa69 to
bb993ca
Compare
|
This PR changed up a bit, but it has the updates from discussion had offline. The tl;dr of those conversations was to introduce a |
jviau
left a comment
jviau
left a comment
There was a problem hiding this comment.
Please address all the warnings from adding [Obsolete] to the version property.
Signed-off-by: halspang <[email protected]>
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
Updated the warnings for Obsolete/changed the reference where appropriate.
| => options is SubOrchestrationOptions derived ? derived.InstanceId : null; | ||
| string instanceId = GetInstanceId(options) ?? this.NewGuid().ToString("N"); | ||
| string defaultVersion = this.invocationContext.Options?.Versioning?.DefaultVersion ?? string.Empty; | ||
| string version = options is SubOrchestrationOptions subOptions ? subOptions.Version : defaultVersion; |
There was a problem hiding this comment.
I just want to leave a comment regarding what we discussed offline here. I don't think it is a good idea to conflate worker version and orchestration version. I see these as separate concerns.
Worker version is purely for routing and supporting rolling updates.
Orchestration version is for enabling an API-versioning-esque scheme for backcompat.
Having worker version apply as a default to orchestration version unnecessarily complicates that scenario. Now customers need to think about how their individual orchestrations version in relation to their worker version as a whole. Which one takes precedence? How does a worker version less than an orchestration version behave? What if they aren't comparable at all?
All of these questions will need to be considered, answered, and documented if we keep these intertangled. Whereas if we just keep them entirely separate then answering all of that becomes much simpler.
There was a problem hiding this comment.
I appreciate the comment and the conversation we had offline about this! I'll make sure that this is all clearly documented/explained.
Having worker version apply as a default to orchestration version unnecessarily complicates that scenario. Now customers need to think about how their individual orchestrations version in relation to their worker version as a whole. Which one takes precedence? How does a worker version less than an orchestration version behave? What if they aren't comparable at all?
The default version only really applies to sub-orchestrations from a Worker point of view. It's matching the client side's default version. In my opinion, the worker shouldn't be starting sub-orchestrations without the client but since it does, we provide a means of setting it. For functions apps, this is the same host.json value. For apps just using this SDK, it does look a little weird as you have to supply it in both the client and the worker options.
In regard to what takes precedence, it's always the version set by the options when scheduling an orchestration. The worker version is only used for the matching/accepting of an orchestration. The way they interact is set with the version matching strategy and the version failure strategy. The docs aren't published to learn yet as we wanted support across the SDKs and DTFx first, but this sample readme talks about the interaction: Orchestration versioning sample.
There was a problem hiding this comment.
Not sure if this was covered in the offline discussion, but it's probably best to not think of or describe this as a "worker version", but rather as a "default orchestration version" which happens to be something that can be configured at a client and/or worker level.
| /// <summary> | ||
| /// Gets the version to associate with the sub-orchestration instance. | ||
| /// </summary> | ||
| public TaskVersion Version { get; init; } = default!; |
There was a problem hiding this comment.
Have we evaluated versioning for TaskActivity? DurableTask.Core has it. I personally haven't used it much myself, unlike orchestration versioning which I used extensively. Still wondering if it is planned?
There was a problem hiding this comment.
I also haven't used it much, the initial thought here is that differently versioned orchestrations can have the different activities and different version activities in the same orchestration were less likely.
That being said, we've left the door open for it, so if it's a requested feature or something we agree to as a team we can still implement it down the road.
3 participants
This commit adds an explict set for Version in TaskName. It also includes a flag that is set so other code can determine if the TaskName is intended to be versioned. Finally, it updates the Version property in the TaskOrchestrationContext as that was always set to empty instead of being able to be used.