search

microsoft / durabletask-mssql

Microsoft SQL storage provider for Durable Functions and the Durable Task Framework
MIT License
87 stars 32 forks source link

add FetchParentInstancesOnly filter to fetch only parent instances if… #99

Closed hsnsalhi closed 2 years ago

hsnsalhi commented 2 years ago

Hello, Refering to my issue at this link https://github.com/Azure/durabletask/issues/733 I created a Pull Request to filter results and retrieve only parent instances This feature is required for our team and hope it'll be helpful for others. I will create an other Pull Request in durabletask to add the property FetchParentInstancesOnly to OrchestrationQuery. Thanks

cgillum commented 2 years ago

I would be fine with this PR if it could be done without requiring any changes to the DurableTask.Core dependency. Otherwise, we will require that this can be implemented for other major storage providers, particularly DurableTask.AzureStorage.

hsnsalhi commented 2 years ago

Hello Chris and thank you for your reply I agree with you but the filter class is defined in DurableTask, this is why I created the PR for that repo (https://github.com/Azure/durabletask/pull/740) to add this property and maybe contribute on the other providers to implement the same feature.

PatriceOngla commented 2 years ago

Hello Chris, I understand your concern but this feature is really useful and even necessary for a large number of scenarios. Maybe a good trade-off could be to implement it first for the sql server provider only and to clearly state in the documentation that the functionality is not yet available with the Azure Storage provider. We can implement it later on this clean basis. What do you think ?

cgillum commented 2 years ago

@PatriceOngla I'm fine with starting with just the SQL provider and then making it work more generically for other providers later. That would be the most expedient way to get this feature out. If you can update this PR to not rely on changes to DurableTask.Core and to also add some integration tests, then I can go ahead and accept this PR and include it in the next release.

PatriceOngla commented 2 years ago

Hello Chris, Thank you for your answer. I can't really figure out how to implement this feature pretty much cleanly without modifying DurableTask.Core (the OrchestrationQuery class). Why not just throw a NotSupportedException for providers that do not implement this feature yet? This is already done for some other features (for example the TaskHubName criterion which triggers a NotSupportedException for the SqlServer provider if I remember well).

cgillum commented 2 years ago

A SQL-only backend change would probably be to the SqlOrchestrationService.GetManyOrchestrationsAsync method. That shouldn't require any changes to DurableTask.Core.

DurableTask.AzureStorage accounts for >99.9% of usage right now. I don't want to add abstractions to DurableTask.Core which aren't supported by the mainstream implementation. I wouldn't mind if it were the other way around, i.e., new abstractions that are initially only supported by DurableTask.AzureStorage. In this case, however, I think it makes much more sense to start with this SQL backend since these kinds of queries will be significantly more efficient on a relational database that can support multiple indexes. This kind of filter will be incredibly slow and expensive to implement on Azure Storage.

hsnsalhi commented 2 years ago

hello, As suggested, let's start with just the SQL provider. I removed the required changes to DurableTask.Core.

hsnsalhi commented 2 years ago

For the failing test, I don't think it has anything to do with my PR, does it ?