Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add versioning support(phase1) #2799

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

skyao
Copy link

@skyao skyao commented Apr 22, 2024

TBD

Issue describing the changes in this PR

resolves #issue_for_this_pr

webjobs updates

  • IDurableOrchestrationClient

    Add override StartNewAsync() method to set instanceVersion.

  • DurableClient

    Update StartNewAsync() method to set instanceVersion correctly to call TaskHubClient.CreateOrchestrationInstanceAsync() method

  • LocalGrpcListener

    Update to call IDurableOrchestrationClient.StartNewAsync() method with version from CreateInstanceRequest.

  • src\WebJobs.Extensions.DurableTask\Microsoft.Azure.WebJobs.Extensions.DurableTask.xml

    Auto update for new added override StartNewAsync() method with instanceVersion parameter.

worker updates

  • FunctionsOrchestrationContext

    Implemented new added 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;
          ......
        }

    The implementation of TaskOrchestrationContext above is FunctionsOrchestrationContext.

dependencies

This PR is depended on following PRs

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
…l TaskHubClient.CreateOrchestrationInstanceAsync() method

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
…h version from CreateInstanceRequest

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

Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
Signed-off-by: Sky Ao <aoxiaojian@gmail.com>
/// <exception cref="ArgumentException">
/// The specified function does not exist, is disabled, or is not an orchestrator function.
/// </exception>
Task<string> StartNewAsync<T>(string orchestratorFunctionName, string instanceId, string instanceVersion, T input);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding a new method without a default implementation to an existing interface is a breaking change, which we can't allow. We will need to find a way to add this overload without introducing a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

Can we change this method to a default method of interface, Like:

        Task<string> StartNewAsync<T>(string orchestratorFunctionName, string instanceId, string instanceVersion, T input) {
          return StartNewAsync(orchestratorFunctionName, instanceId, input);
        }

But this need runtime to support default interface implementation:

C:\Users\sky\work\code\durabletask-fork2\azure-functions-durable-extension\src\WebJobs.Extensions.DurableTask\ContextInterfaces\IDurableOrchestrationClient.cs(182,22): erro 
r CS8701: Target runtime doesn't support default interface implementation. [C:\Users\sky\work\code\durabletask-fork2\azure-functions-durable-extension\src\WebJobs.Extension 
s.DurableTask\WebJobs.Extensions.DurableTask.csproj::TargetFramework=netstandard2.0]

Copy link
Author

Choose a reason for hiding this comment

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

Another solution is trying to reuse orchestratorFunctionName, we can add version to this parameter with a predefined seperator(using @ as example, we can make desicion later if we accept this solution) :

orchestratorFunctionName="HelloOrchestrator"
orchestratorFunctionName="HelloOrchestrator@1.2.0"

In the implementation, we can try to split orchestratorFunctionName to get real orchestratorFunctionName and optional version:

orchestratorFunctionName="HelloOrchestrator" --> orchestratorFunctionName="HelloOrchestrator", version=""
orchestratorFunctionName="HelloOrchestrator@1.2.0" --> orchestratorFunctionName="HelloOrchestrator", version="1.2.0"

This solution should work fine without breaking change, but it's just a bit ugly.

Copy link
Contributor

@jviau jviau Apr 26, 2024

Choose a reason for hiding this comment

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

@ is already used for entity support and has some existing semantics to it. And if we do go with some special character separator, we would have to consider the impact to existing instance IDs that might already contain that character.

Adding support here is challenging without a breaking change. The simplest route would be a new interface IDurableOrchestrationClient2 and we add it there.

There is also another option: don't add support for this for in-proc. Add it to out of proc only.

Copy link
Author

Choose a reason for hiding this comment

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

we can change @ to something else like ||.

This combine of orchestratorFunctionName and version will only be used to call this StartNewAsync() method:

  • Before calling StartNewAsync, there are two separated variables: orchestratorFunctionName and version, we just combine them as "orchestratorFunctionNam||version" to pass them into StartNewAsync() method with one orchestratorFunctionName paremeter.

  • In StartNewAsync() method, we will split orchestratorFunctionName paremeter by || at first and get back real orchestratorFunctionName and version, then "orchestratorFunctionName||version" won't use any more.

Copy link
Author

Choose a reason for hiding this comment

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

@cgillum @jviau would you please have a look at this PR and make a decision to make it work without breaking changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's go with @jviau's suggestion, and NOT add support for versioning in the WebJobs APIs. These WebJobs APIs are already moving towards deprecation. Let's instead focus on just the APIs defined in microsoft/durabletask-dotnet. If you agree, then we can close this PR.

Copy link
Author

Choose a reason for hiding this comment

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

I agree.

Yet This PR is used by me in the updated quickstart to show how versioning works in durable-functions, as well as:

https://learn.microsoft.com/en-us/azure/azure-functions/durable/durable-functions-create-first-csharp?pivots=code-editor-vscode#add-functions-to-the-app

Without this PR, I can't how an end-to-end process to do versioning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants