Allow TaskHubClient's CreateOrchestrationInstanceAsync to schedule orchestrators

[davidmrdavid]

Related: Azure/azure-functions-durable-extension#2805

In the PR above, we learned if a user wants to schedule an orchestrator to start a particular time, they need to use the CreateScheduledOrchestrationInstanceAsync instead of using the more commonly used CreateOrchestrationInstanceAsync API.

Allowing CreateOrchestrationInstanceAsync to also schedule orchestrators by accepting a new fireAt would simplify things downstream (in durabletask-dotnet, it would simplify the PR linked above^) so this PR adds that "missing" parameter. This does create some repetition (there are now two APIs that allow users to schedule orchestrators) but I think it makes the orchestrator-scheduling feature more discoverable while also helping simplify downstream code.

This PR also include 2 additional minor contributions:

1. It enables nullable analysis in TaskHubClient, and performs the minimal edits needed to get the analysis to pass.
2. It adds a simple TaskHubClient test validating that the class is able to schedule orchestrators to fire at a given time.

[cgillum]

If another method already exists that implements the scheduling behavior, why is this PR needed? In other words, why can’t we have the consuming SDK use the existing method? That wasn’t clear in this PR description and I’m hesitant to agree to adding a redundant method overload.

[davidmrdavid]

Yeah I didn't make this clear. I see two main reasons to make this change, but please do push back if needed:

First - The overloads for CreateOrchestrationInstanceAsync accept a strictly larger number of parameters than CreateScheduledOrchestrationInstanceAsync. Therefore, durabletask-dotnet can't simply call the CreateScheduledOrchestrationInstanceAsync without possibly dropping parameters like dedupStatuses and tags.

For evidence: if we do a CTRL+F for "Task CreateScheduledOrchestrationInstanceAsync" in TaskHubClient you'll see that there's only 3 matches:

(A.1)

durabletask/src/DurableTask.Core/TaskHubClient.cs

Lines 83 to 86 in 7a778c3

	public Task<OrchestrationInstance> CreateScheduledOrchestrationInstanceAsync(
	Type orchestrationType,
	object? input,
	DateTime startAt)

(A.2)

durabletask/src/DurableTask.Core/TaskHubClient.cs

Lines 108 to 113 in 7a778c3

	public Task<OrchestrationInstance> CreateScheduledOrchestrationInstanceAsync(
	Type orchestrationType,
	string? instanceId,
	object? input,
	DateTime startAt)
	{

(A.3)

durabletask/src/DurableTask.Core/TaskHubClient.cs

Line 135 in 7a778c3

public Task<OrchestrationInstance> CreateScheduledOrchestrationInstanceAsync(string name, string version, string? instanceId, object? input, DateTime startAt)

Meanwhile, a CTRL+F for Task<OrchestrationInstance> CreateOrchestrationInstanceAsync yields 9 results, too many to list here. At it's largest parameter set, this method accepts up to 7 parameters, compared to the 5 parameters listed in (A.3), see here and notice the missing params relative to (A.3):

durabletask/src/DurableTask.Core/TaskHubClient.cs

Lines 344 to 351 in 7a778c3

	public Task<OrchestrationInstance> CreateOrchestrationInstanceAsync(
	string name,
	string version,
	string? instanceId,
	object? input,
	IDictionary<string, string> tags,
	OrchestrationStatus[] dedupeStatuses,
	DateTime startAt)

So, it seems to me that there's a parameter-parity gap between the two APIs that favors CreateOrchestrationInstanceAsync. That makes me think it would be better, maintenance-wise, for CreateOrchestrationInstanceAsync to fully subsume the behavior of CreateScheduledOrchestrationInstanceAsync by giving it that missing (fireAt parameter). The alternative is giving CreateScheduledOrchestrationInstanceAsync more overloads.

Second - CreateOrchestrationInstanceAsync can actually already schedule orchestrations, see the last parameter of this overload for evidence:

durabletask/src/DurableTask.Core/TaskHubClient.cs

Line 175 in 7a778c3

public Task<OrchestrationInstance> CreateOrchestrationInstanceAsync(Type orchestrationType, object? input, DateTime startAt)

So it seems to me that CreateOrchestrationInstanceAsync was already intended to subsume CreateScheduledOrchestrationInstanceAsync, but that we're missing an overload that allows us to set fireAt in addition to all the other parameters (the 7 I mentioned above). This PR adds that missing overload.

Hope that makes sense, sorry for the wall of text. In short - I think the two APIs already have overlapping responsibilities, and I'm just trying to give the API with more parameters another overload to have it fully subsume the other one.

[davidmrdavid]

Ultimately though, I don't feel strongly. Just think this makes sense in the long run.

[cgillum]

@davidmrdavid got it, the two reasons you called out have eased my concerns. I'm good with these changes. Thanks for explaining!

[cgillum]

Since this file is the only place where this new class is being used, can it instead be an inner class of TaskHubClientTests instead of being a top-level class?

[davidmrdavid]

Sorry, I completely missed this and I already just merged the PR. I'll add this to my ToDo list for a future iteration