Support 202 Automatic Polling at Worker.Extensions.DurableTask #3092
Conversation
| } | ||
| else | ||
| { | ||
| // TODO: use the configuration DefaultAsyncRequestSleepTimeMilliseconds from durabletaskextension |
There was a problem hiding this comment.
The context here is from TaskOrchestrationContext, and it doesn't have property saving config from DurableExtension. We can do this but that might require quite a lot changes, including DurableTask.Abstractions. Thus I am not sure if we should do that, or if there is any other easy way to do that.
There was a problem hiding this comment.
Hmm. I'm not sure the right approach for this. One option would be to introduce a generic property bag (Dictionary<string, object>) in TaskOrchestrationContext, and have that be populated using data from the trigger context that we get from the WebJobs extension. For example, adding a a key named df.http.defaultAsyncRequestSleepTimeMilliseconds and have the value come from host.json. We still have to add the property bag to the DurableTask.Abstractions, but we only need to make that one change and we can easily add other properties to the dictionary in the future without changing DurableTask.Abstractions a second time. Let me know if I can help explain this idea better.
| public class HTTPFeatureTests | ||
| { | ||
| private readonly FunctionAppFixture _fixture; | ||
| private readonly ITestOutputHelper _output; |
There was a problem hiding this comment.
Looks like this code is not following our style guidelines. For example:
| private readonly ITestOutputHelper _output; | |
| readonly ITestOutputHelper output; |
Please update accordingly.
| Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); | ||
| string statusQueryGetUri = await DurableHelpers.ParseStatusQueryGetUriAsync(response); | ||
|
|
||
| await DurableHelpers.WaitForOrchestrationStateAsync(statusQueryGetUri, "Completed", 150); |
There was a problem hiding this comment.
What is the 150 parameter and what are it's units? Please use a named parameter to make the code easier to understand.
|
|
||
| // Verify that the output contains the LongRunningOrchestrator's result, | ||
| // ensuring the orchestrator completed and did not just return a 202 Accepted. | ||
| Assert.Contains("Hello Tokyo", orchestrationDetails.Output); |
There was a problem hiding this comment.
We should also assert the the orchestration ran for the expected length of time.
Also, is there any way we can confirm how many times the implementation did polling, perhaps by examining logs?
There was a problem hiding this comment.
Do we emit any logs about the activity named HttpTaskActivityReservedName?
| // If Headers is null or missing, we can't poll the Location URL, so return the response. | ||
| if (response.Headers is null) | ||
| { | ||
| break; |
There was a problem hiding this comment.
Can we add a log here that will let us know that the headers were null or missing? It might help with debugging.
|
|
||
| // Verify that the output contains the LongRunningOrchestrator's result, | ||
| // ensuring the orchestrator completed and did not just return a 202 Accepted. | ||
| Assert.Contains("Hello Tokyo", orchestrationDetails.Output); |
There was a problem hiding this comment.
Do we emit any logs about the activity named HttpTaskActivityReservedName?
| // TODO: use the configuration DefaultAsyncRequestSleepTimeMilliseconds from durabletaskextension | ||
| fireAt = context.CurrentUtcDateTime.AddMilliseconds(30000); | ||
| // Gets configuration DefaultAsyncRequestSleepTimeMilliseconds from durabletaskextension | ||
| int defaultAsyncRequestSleepTimeMilliseconds = context.Properties.TryGetValue("df.http.defaultAsyncRequestSleepTimeMilliseconds", out var value) && value is double d |
There was a problem hiding this comment.
Can you extract this to a constant? Same with the other location of this. I get they are different assemblies with no common library to put it in - so two different constants are fine.
| <add key="nuget.org" value="https://api.nuget.org/v3/index.json" /> | ||
| <add key="Package source" value="C:\Users\naiyuantian\localNuget" /> |
There was a problem hiding this comment.
Make sure to remove this before checking in.
| @@ -39,6 +43,9 @@ public RemoteOrchestratorContext(OrchestrationRuntimeState runtimeState, TaskOrc | |||
| [JsonProperty("upperSchemaVersion")] | |||
| internal int UpperSchemaVersion { get; } = 4; | |||
|
|
|||
| [JsonProperty("configurations")] | |||
| public Dictionary<string, object?> Configurations { get; private set; } | |||
There was a problem hiding this comment.
This doesn't have to be a dictionary - you could have a POCO here. It will serialize all the same.
Issue describing the changes in this PR
resolves #2781
CallHttpAsyncat TaskOrchestrationContextExtensionMethods.cs to support automatic polling of 202 responsesPropertiesto TaskOrchestrationContext microsoft/durabletask-dotnet#415 is released.Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.