-
Notifications
You must be signed in to change notification settings - Fork 272
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
APIs to enumerate all orchestration instances #323
Conversation
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.
This is looking great! I'm very excited about this feature. I would like to request a few small changes to make sure the performance remains good.
Also, I think it would be a good idea to create a built-in REST API that can also invoke this. You can add it to HttpApiHandler.cs, just like we do for the other GetStatus methods.
Lastly, let's make sure to add some tests for this new feature.
var tasks = new List<Task<DurableOrchestrationStatus>>(); | ||
foreach (var state in states) | ||
{ | ||
tasks.Add(this.GetDurableOrchestrationStatusAsync(state, showHistory, showHistoryOutput)); |
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 worry that this could cause thread-pool starvation because we're potentially starting an unlimited number of tasks in parallel. I looked at the implementation of GetDurableOrchestrationStatusAsync
, and it appears that it only does async work if showHistory
is true. Otherwise, it completes quickly and synchronously. To avoid risk, can we remove showHistory
and showHistoryOutput
from this new API and set it always to false
when calling GetDurableOrchestrationStatusAsync
? I think getting the history of every instance at the same time will not be necessary for the DevOps scenarios we are interested in, and this will make sure we do not explode our thread usage.
If we make this suggested change, then I think you can get rid of Task.WhenAll
and just add items to the list one at a time.
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.
Thank you, Make sense. I push some changes. One of the problem is GetDurableOrchestrationStatusAsync
has too much responsibilities. I extract the conversion part. It doesn't have async/await
operation. Then it might help people understand the code more.
// TODO this cast is to avoid to change DurableTask.Core. Change it to use TaskHubClient. | ||
var serviceClient = (DurableTask.AzureStorage.AzureStorageOrchestrationService)this.client.serviceClient; | ||
var states = await serviceClient.GetOrchestrationStateAsync(cancellationToken); | ||
var tasks = new List<Task<DurableOrchestrationStatus>>(); |
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.
By default, the Task<T>
class will create a list size of some small number, maybe 4 or 11, I forget. If we add more items than this, Task<T>
will silently discard its internal buffer, create a new one of double size, and copy all the items from the first buffer to the new buffer. It will keep doing this until we finish adding elements to the list. This is very inefficient and can cause performance problems with the .NET Garbage Collector. The best practice is to use the List<T>(int)
constructor overload to specify an initial list capacity that meets our needs. In this case, I think you should say the following:
var result = new List<DurableOrchestrationStatus>(states.Count);
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.
Oh, no. I didn't know that. That is why people use List(int) ... I double check the spec. However, this code has gone. :) Now no problem.
/// </summary> | ||
/// <param name="showHistory">Boolean marker for including execution history in the response.</param> | ||
/// <param name="showHistoryOutput">Boolean marker for including input and output in the execution history response.</param> | ||
/// <param name="cancellationToken">Cancel your request by this token</param> |
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 would phrase this token a little differently to match the passive wording style of the other parameter descriptions. For example:
Cancellation token that can be used to cancel the status query operation.
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.
Thanks. Done it.
Hi @cgillum , I wrote code for HttpApiHandler and HttpApiHandlerTest. I have two questions for the architecture.
I intend that the url might be /instances/ with Get request, it returns all instances. It works, however, it is not intuitive on the code.
|
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.
Regarding your questions:
- Yes, adding
InstanceId
is great - thanks for doing this. - Yes, I think this is necessary.
I apologize that the HTTP request path handling is not very simple. It might be over-optimized for performance.
I left some feedback on the code. In general this looks very good and I think we're close to merging it. Let me know if your tests are passing locally and we can go ahead and get started on the DurableTask.AzureStorage publishing.
} | ||
catch (Exception e) | ||
{ | ||
return request.CreateErrorResponse(HttpStatusCode.InternalServerError, "Internal Server Error", e); |
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 believe this is the default behavior, so no need to have this try/catch.
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.
Done
@@ -104,6 +105,13 @@ public HttpApiHandler(DurableTaskExtension config, ILogger logger) | |||
int i = path.IndexOf(InstancesControllerSegment, StringComparison.OrdinalIgnoreCase); | |||
if (i < 0) | |||
{ | |||
// Retrive All Status in case of the request URL ends e.g. /instances/ | |||
if (request.Method == HttpMethod.Get | |||
&& path.IndexOf(InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase) >= 0) |
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.
Not sure if this is 100% correct. Does GET /instances12345
also resolve to this API? What about GET /FooBarInstances
? Maybe you can simply do an equality check.
if (request.Method == HttpMethod.Get &&
path.Equals(InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase)
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.
Sorry about that. I fixed that.
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've done that.
@@ -115,7 +123,7 @@ public HttpApiHandler(DurableTaskExtension config, ILogger logger) | |||
string instanceId = path.Substring(i); | |||
if (request.Method == HttpMethod.Get) | |||
{ | |||
return await this.HandleGetStatusRequestAsync(request, instanceId); | |||
return await this.HandleGetStatusRequestAsync(request, instanceId); |
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.
Looks like some extra whitespace got added?
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.
Done!
try | ||
{ | ||
DurableOrchestrationClientBase client = this.GetClient(request); | ||
var status = await client.GetStatusAsync(); |
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.
In cases where the type of the variable is not obvious, I prefer we use the actual type name instead of var
. It makes the code more easily understandable to others. In this case, I would prefer:
List<DurableOrchestrationStatus> statusForAllInstances = await client.GetStatusAsync();
I apologize that the convention is not consistently used everywhere.
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.
Sorry about that. I should have notice that. For statusForAllInstances
naming is good to know! I was wondering which is the good naming for this. Done!
test/HttpApiHandlerTests.cs
Outdated
Method = HttpMethod.Get, | ||
RequestUri = getStatusRequestUriBuilder.Uri, | ||
}); | ||
var actual = JsonConvert.DeserializeObject<IList<StatusResponsePayload>>(await responseMessage.Content.ReadAsStringAsync()); |
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.
Before this, I suggest you add a check for HTTP 200 status code.
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.
Done!
/// InstanceId. | ||
/// </summary> | ||
[DataMember(Name = "instanceId")] | ||
public JToken InstanceId { get; set; } |
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.
This should be string
, not JToken
.
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.
Done!
Hi @cgillum , I've done all. |
@@ -32,6 +28,11 @@ | |||
<Compile Include="..\..\.stylecop\GlobalSuppressions.cs" Link="GlobalSuppressions.cs" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="DurableTask.AzureStorage" Version="1.2.2" /> | |||
<PackageReference Include="DurableTask.Core" Version="2.0.0.5" /> |
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's not necessary to reference DurableTask.Core explicitly. Referencing DurableTask.AzureStorage is sufficient.
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.
Done
@@ -33,4 +33,8 @@ | |||
<Compile Include="$(SolutionDir)\.stylecop\GlobalSuppressions.cs" Link="GlobalSuppressions.cs" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" /> |
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.
Do you know what this is?
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 don't know. It is not my intent. When I search the internet ... element, I found it is a notation to tell vs that this project is test project with 3rd party library.
https://stackoverflow.com/questions/18614342/what-is-service-include-in-a-csproj-file-for
Shall we remove this or keep this?
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.
Since it wasn't there originally and you didn't add it intentionally, I would remove it.
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.
Removed!
if (request.Method == HttpMethod.Get | ||
&& path.IndexOf(InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase) >= 0) | ||
&& string.Equals(path?.Substring(indexOfSegment), InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase)) |
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.
Isn't it simpler to just say string.Equals(path, InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase)
? I don't think you need the indexOfSegment
variable.
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.
Why we need this is, path is not "/Instances/". Actually it is "/admin/extensions/DurableTaskExtension/Instances". Since the InstancesControllerSegment is "/Instances/" we need to do Substring it.
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.
Oh, you're right! Sorry - my bad. Would path.EndsWith(InstancesControllerSegment.TrimEnd('/'), StringComparison.OrdinalIgnoreCase)
work? If so, that might be a little more efficient.
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 is much cooler than my code. :) Thanks! Done
Hi @cgillum , |
I responded to some of your comments, but generally I think this looks ready. If everything is working for you locally, let's go ahead and merge your DurableTask.AzureStorage changes in the durabletask repo. I will publish the nuget package to our staging location, and then you can update this PR to reference the newest version. That should allow the CI to start working. If the CI succeeds, then I will go ahead and publish the latest DurableTask.AzureStorage to nuget.org and we can merge this PR. |
OK, a package with your DurableTask.AzureStorage change has been added here: https://www.myget.org/feed/azure-appservice/package/nuget/DurableTask.AzureStorage. Can you please update your reference to point to 1.2.3? Our CI should be searching myget for packages automatically. |
Hi @cgillum , One question, when I build locally, It requires DurableTask.Core 2.0.0.6. (I tried DurableTask.Core 2.0.0.5 however, it doesn't work). I remember that you said DurableTask.Core is not required. Do I missing something? |
It works on the CI. However, not for my local. lol. If I add DurableTask.Core 2.0.0.6 it might work. Unless the DurableTask.Core, I have this error. Is it some kind of cache issue?
|
According to the link I shared previously, DurableTask.Core 2.0.0.5 is the required dependency. There is no 2.0.0.6. Did you add a NuGet package source for https://www.myget.org/F/azure-appservice/api/v3/index.json in your Visual Studio? You need to add that in order to download the new package locally. |
I have it on the NuGet.config and Package source. I'll ask @gled4er about it tomorrow.
|
Hi @cgillum , Do we need some push to start full-ci? |
The full CI already ran and is already green. Were you able to get everything working locally? |
Hi @cgillum No only my local machine has problem. I've done a lot of work around, however, I can't solve this problem. When I build it I've got this error.
I changed a version of VS, remove bin/obj directory, or remove When I see the |
Hi @cgillum , I finally figured out why it happens. I left the old DurableTask.Core on my LocalNuGet dir. Maybe it is precedent than the network repo. |
It's odd that it works in the CI but not locally. Could it be related to the order in which it looks for packages across different package sources on your machine? For example, if you list nuget.org first before myget, does it work? I suspect the problem could be that DurableTask.AzureStorage is looking for a DurableTask.Core with a strong name, but maybe there is a version of DurableTask.Core myget which doesn't have a strong name. I've seen similar build problems in the past caused by this. |
Oh, glad you figured it out! |
Hi @cgillum , I tested this branch locally. It works! |
@cgillum |
Hi @cgillum |
I think we've covered everything! The only caveat is that the required DurableTask.AzureStorage v1.2.3 package is currently only in MyGet, and not in nuget.org. I will move it to nuget.org a little bit later if that's not a problem. Thanks for this awesome PR!! This one was tricky because it involved changes across two different repos, but I'm glad we got through it. |
Thank you for the review. I enjoyed a lot. I'll go on the next PR. lol |
Requires DurableTask.AzureStorage v1.2.3 or greater
Hi @cgillum
I start this pull request to start conversation.
This pull request is for this issue.
#316
Also, related this pull request for DurableTask
Azure/durabletask#187
NOTE
If you do CI for this pull request, currently it must fail. This fix need the code change of the durable task pull request. I write a code with the nuget packages of DurableTask.AzureStorage (2.0.0.5) I don't include csproj file since it is changed to load local file.
I have some questions for you.
Anyway, this is the first steps. :) Please review this before go deep. :)