-
Notifications
You must be signed in to change notification settings - Fork 273
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
Durable Client Factory (DurableOrchestrationClient outside Azure Functions ) #1125
Conversation
/// </summary> | ||
/// <param name="durableClientOptions">options containing the client configuration parameters.</param> | ||
/// <returns>Returns a <see cref="IDurableClient"/> instance. The returned instance may be a cached instance.</returns> | ||
public IDurableClient CreateClient(DurableClientOptions durableClientOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also could be CreateExternalClient and remove the Boolean property and set always true in the attribute to avoid this overhead, I thought it also can be used inside an azure function somewhere not only from external applications.
I like this idea of having a first-class way to initialize an It might be useful to see a few more examples, such as the ASP.NET Core setup required to use a non-default task hub name and Azure Storage account. |
This is great. Thanks! I wonder if we should publish this as a separate package that references |
@cgillum ok will do also I think it might be a good idea allow configure IOption to directly configure it in the appsetting.json. What are you namespace suggestions to clean up the code ? I've only created the factory under
|
@anthonychu great thanks ! :) you mean something like |
Sorry guys I was out for the holiday season of new year, will address this during the week to get a new version more friendly with @cgillum suggestions. |
I've tried to use a connectionstring instead for configuration but to do that we should changes several providers so I still think the best way is keep configure the connection name and read it from configurations. With changes I made it's easy to configure in the asp.net app and we can call create client w/o params as well have the flexibility to call different clients if any need to reach different durable functions. public class Startup
{
public Startup(IConfiguration configuration)
{
Configuration = configuration;
}
public IConfiguration Configuration { get; }
public void ConfigureServices(IServiceCollection services)
{
services.AddMvc().SetCompatibilityVersion(CompatibilityVersion.Version_2_2);
services
.AddDurableTask(options =>
{
options.ConnectionName = "Default";
options.TaskHub = "name";
});
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
if (env.IsDevelopment())
{
app.UseDeveloperExceptionPage();
}
else
{
app.UseHsts();
}
app.UseHttpsRedirection();
app.UseMvc();
}
} then to use is extremely straight forward. [Route("api/[controller]")]
[ApiController]
public class ValuesController : ControllerBase
{
private readonly IDurableClientFactory _clientFactory;
public ValuesController(IDurableClientFactory clientFactory)
{
_clientFactory = clientFactory;
}
[HttpGet]
public async Task<IActionResult> Get()
{
var client = _clientFactory.CreateClient();
var instanceId = await client.StartNewAsync("Chaining");
return Ok(instanceId);
}
} and if any one want to reach a specific azure durable function who is not the default configured in the startup (it will be only for complicated and long structures) the code have the flexibility to call like this. [HttpGet]
public async Task<IActionResult> Get()
{
var client = _clientFactory.CreateClient(new DurableClientOptions
{
ConnectionName = "InvoicingAppStorage",
TaskHub = "InvoicingHub"
});
var instanceId = await client.StartNewAsync("Chaining");
return Ok(instanceId);
} Finally I agree with @anthonychu to move:
outside of this project and create some like Let me know your thoughts, I'm very open to do any changes you think it will be necessary and looking forward to collaborate more often in the future :). |
any thoughts ? |
Sorry for the delay here. I am going to try to review this in the next day or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave some initial rounds of feedback. I think we will need to do a bit of refactoring to ensure we don't need to import all of the services, when we really only need a few to properly create the functionality of DurableClient
in external/web apps.
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClientFactory.cs
Show resolved
Hide resolved
@@ -106,7 +106,10 @@ async Task<IActionResult> IDurableOrchestrationClient.WaitForCompletionOrCreateC | |||
/// <inheritdoc /> | |||
async Task<string> IDurableOrchestrationClient.StartNewAsync<T>(string orchestratorFunctionName, string instanceId, T input) | |||
{ | |||
this.config.ThrowIfFunctionDoesNotExist(orchestratorFunctionName, FunctionType.Orchestrator); | |||
if (!this.attribute.ExternalClient) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to do something similar in SignalEntityAsyncInternal
.
this.config.ThrowIfFunctionDoesNotExist(orchestratorFunctionName, FunctionType.Orchestrator); | ||
if (!this.attribute.ExternalClient) | ||
{ | ||
this.config.ThrowIfFunctionDoesNotExist(orchestratorFunctionName, FunctionType.Orchestrator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgillum, is there any reason for us to have this check at all? A DurableClient
can be created with a different task hub already, and there is no guarantee that the app that is calling this DurableClient
will have the same functions as the app with the TaskHub the client is referencing.
src/WebJobs.Extensions.DurableTask/DurableTaskJobHostConfigurationExtensions.cs
Show resolved
Hide resolved
/// <param name="serviceCollection">The <see cref="IServiceCollection"/> to configure.</param> | ||
/// <param name="optionsBuilder">Populate default configurations of <see cref="DurableClientOptions"/> to create Durable Clients.</param> | ||
/// <returns>Returns the provided <see cref="IServiceCollection"/>.</returns> | ||
public static IServiceCollection AddDurableTask(this IServiceCollection serviceCollection, Action<DurableClientOptions> optionsBuilder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method should be AddDurableTaskFactory()
, and honestly might be better served in it's own namespace/package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed elsewhere in this review, we will keep it in this namespace/package.
There are questions to be brought up about other function apps that don't want to use Durable except for the external client and how we support that, as the functions runtime will automatically start up an instance of the extension, but that can be addressed later.
@bachuv, we may still want to rename this before the v2.4.0 release.
src/WebJobs.Extensions.DurableTask/DurableTaskJobHostConfigurationExtensions.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClientFactory.cs
Outdated
Show resolved
Hide resolved
@ConnorMcMahon thanks for your feedback I'll do the proper changes shortly. Also do you want I create the separete project to put the IOC and ClientFactory as suggested ? any recommendation for naming it ? |
Personally I'd prefer to not create a separate package. I think the reason to have a separate package is if that package contained additional dependencies that don't make sense in the core Microsoft.Azure.WebJobs.Extensions.DurableTask. It doesn't look like you're adding any new dependencies, so I vote that we keep this all in the same package. |
Hey @davidrevoledo were you still interested in completing this PR? I was hoping to include this in our v2.3.0 release, but we're wrapping that up now and I see there are some merge conflicts that need to be resolved. I'd be happy to include this in our v2.4.0 release if you're up to finishing it. |
sure thing @cgillum , I've been so busy sorry to forget this PR, I've resolved the conflict but let me check if I've changed all the mentioned points in here. One thing to let you know is that I needed to call durable functions from a asp.net core api so I've been using this for a while, will be great to delete the internal code and having from the library itself :D |
a6d5f36
to
b189084
Compare
@davidrevoledo So sorry for all of the delays getting this work in. We are going to merge in now for our v2.4.0 release, and I know that we will have a lot of very excited customers to use this. We will make sure to give you credit for these new features in our next release. @bachuv is going to help us drive this to production, with some samples of usage in a console app as well as some tests and validation. Let us know if you are interested in being involved as a reviewer in those PRs! |
Not a problem ! @ConnorMcMahon honoured to contribute, is part of my commitment as an Azure MVP Ready here for anything you might need ! |
Will this also allow me to inject |
@olitomlinson, to clarify, do you mean This PR does not explicitly support that, but the good news is that the |
Sorry yes, I do mean ‘IDurableEntityClient’ - I’m having to use the Entity HTTP API from ASP.NET project to signal and read entity state which isn’t terribly easy, so being able to use the durable entity client will be brilliant! |
These changes are for the new Durable Client feature (#1125). With the new feature, there isn't a way to create the HTTP management payloads from the lack of a webhook URL so exceptions are thrown when a user tries to call CreateCheckStatusResponse or CreateHttpManagementPayload.
This is awesome! 🥇 However, what is still missing is documenting this so people know it's possible. I only noticed after a very specific web search and finding this old PR. I would suggest documenting it under the section "Orchestration client" here: |
Ref : #161
This is intended to init azure durable functions outside of an azure function orchestrators host.
It could have extensive applications to init durable functions from different applications event from other azure functions applications.
Usability is straight forward for a client. Let's take an Asp.Net Core application.
To configure the application just call AddDurableTask, this will use service collection instead of webjob extension builder.
To create the client just inject
IDurableClientFactory
under the hood this will create a client using the same approach Orchestrator host is doing to avoid breaking changes, we could configure hub name or the connection string for the storage accountThe implementation on the orchestrator host is exactly the same without any client needed to start the flow.
Let me know your thoughts and will complete a few unit tests required for the external client flag to avoid the validation of know function names.
https://github.com/Azure/azure-functions-durable-extension/pull/1125/files#diff-b4be2ec3bf4e13899ce8445628aa091aR109
Any suggestion to control that validation is welcome.