add multi instance query support logics #702

kaibocai · 2022-04-11T20:59:30Z

add multi instance query support logics refer here microsoft/durabletask-java#9 for more details.
This PR includes

created general query interface in durable core
Implement the query interface in AzureStorageOrchestrationService

kaibocai · 2022-04-11T22:54:50Z

src/DurableTask.Core/Query/IOrchestrationServiceQueryClient.cs

+        /// <param name="condition">Return orchestration instances that match the specified conditions.</param>
+        /// <param name="cancellationToken">Cancellation token that can be used to cancel the status query operation.</param>
+        /// <returns>Returns each page of orchestration status for all instances and continuation token of next page.</returns>
+        Task GetOrchestrationStateWithFiltersAsync(OrchestrationStatusQueryCondition condition, CancellationToken cancellationToken);


should this method have return type generic as

Task<OrchestrationStatusQueryResult> GetOrchestrationStateWithFiltersAsync(OrchestrationStatusQueryCondition condition, CancellationToken cancellationToken);

to align with what we have in durable extension?

Yes. Without this, you're not actually returning anything.

cgillum

This is a good start. I have several suggestions but they are fairly minor.

cgillum · 2022-04-11T23:02:31Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

@@ -0,0 +1,81 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT License. See LICENSE in the project root for license information.


All the code in this repo is under the Apache license. Please copy/paste the file header that you see for some of the other existing code files.

cgillum · 2022-04-11T23:03:24Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+// Licensed under the MIT License. See LICENSE in the project root for license information.
+
+using System;
+using System.Collections.Generic;


The coding convention in this repo is to put the using statements inside the namespace block. See some of the other source files for an example. C# allows you to do either way but I'd like to keep the code as self-consistent as possible.

cgillum · 2022-04-11T23:04:42Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+        /// <summary>
+        /// Determines whether the query will include the input of the orchestration.
+        /// </summary>
+        public bool ShowInput { get; set; } = true;


Let's name this FetchInputsAndOutputs.

cgillum · 2022-04-11T23:07:18Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+        /// <summary>
+        /// Return orchestration instances which matches the TaskHubNames.
+        /// </summary>
+        public IEnumerable<string> TaskHubNames { get; set; }


Let's use ICollection<string> instead of IEnumerable<string>. That will make this API a little easier to use.

cgillum · 2022-04-11T23:08:13Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+        /// <summary>
+        /// Return orchestration instances which matches the runtimeStatus.
+        /// </summary>
+        public IEnumerable<OrchestrationStatus> RuntimeStatus { get; set; }


Let's use ICollection<OrchestrationStatus> instead if IEnumerable<OrchestrationStatus>. That will make this API a little easier to use.

cgillum · 2022-04-11T23:17:35Z

src/DurableTask.Core/Query/IOrchestrationServiceQueryClient.cs

+        /// Gets the status of all orchestration instances with paging that match the specified conditions.
+        /// </summary>
+        /// <param name="condition">Return orchestration instances that match the specified conditions.</param>
+        /// <param name="cancellationToken">Cancellation token that can be used to cancel the status query operation.</param>


Remove the word "status" here.

Suggested change

/// <param name="cancellationToken">Cancellation token that can be used to cancel the status query operation.</param>

/// <param name="cancellationToken">Cancellation token that can be used to cancel the query operation.</param>

cgillum · 2022-04-11T23:18:31Z

src/DurableTask.Core/Query/IOrchestrationServiceQueryClient.cs

+        /// <param name="condition">Return orchestration instances that match the specified conditions.</param>
+        /// <param name="cancellationToken">Cancellation token that can be used to cancel the status query operation.</param>
+        /// <returns>Returns each page of orchestration status for all instances and continuation token of next page.</returns>
+        Task GetOrchestrationStateWithFiltersAsync(OrchestrationStatusQueryCondition condition, CancellationToken cancellationToken);


Yes. Without this, you're not actually returning anything.

cgillum · 2022-04-11T23:20:43Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+        /// </summary>
+        public Task GetOrchestrationStateWithFiltersAsync(OrchestrationStatusQueryCondition condition, CancellationToken cancellationToken)
+        {
+            var transferredCondition = TransferToAzureStorageCondition(condition);


Let's not use var here since we can't immediately see the variable type.

Suggested change

var transferredCondition = TransferToAzureStorageCondition(condition);

OrchestrationInstanceStatusQueryCondition convertedCondition = TransferToAzureStorageCondition(condition);

cgillum · 2022-04-11T23:21:47Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+        //    {
+        //        this.InstanceIdPrefix = "@";
+        //    }
+        //}


Can this commented-out code be removed?

cgillum · 2022-04-11T23:23:24Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+        /// <summary>
+        /// Return orchestration instances which were created before this DateTime.
+        /// </summary>
+        public DateTime CreatedTimeTo { get; set; }


It might be a safer API design to make these two properties DateTime? instead of DateTime. That way it's easier to understand whether we want to include them as filter criteria or not. It's very easy to accidentally do the wrong thing when dealing with default values for DateTime.

cgillum

Looks like some code was copied into this project that doesn't need to be. Some more suggestions below, including some class rename suggestions.

cgillum · 2022-04-12T04:31:44Z

src/DurableTask.Core/Query/OrchestrationRuntimeStatus.cs

+    /// <summary>
+    /// Represents the possible runtime execution status values for an orchestration instance.
+    /// </summary>
+    public enum OrchestrationRuntimeStatus


Is this copied from the Durable Functions extension project? We should not be using this. We should be instead using the existing DurableTask.Core.OrchestrationStatus enum mentioned in the comment on line 17 since that already exists in this assembly. No need to create yet another copy of it.

cgillum · 2022-04-12T04:34:45Z

src/DurableTask.Core/Query/IOrchestrationServiceQueryClient.cs

+//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+//  ----------------------------------------------------------------------------------


It looks like you double-copied the license text?

cgillum · 2022-04-12T04:37:05Z

src/DurableTask.Core/Query/DurableOrchestrationStatus.cs

+    /// <summary>
+    /// Represents the status of a durable orchestration instance.
+    /// </summary>
+    public class DurableOrchestrationStatus


Was this also copied from the Durable Functions extension project? You should instead be using the existing DurableTask.Core.OrchestrationState class.

cgillum · 2022-04-12T04:38:25Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

        }

-        internal OrchestrationInstanceStatusQueryCondition TransferToAzureStorageCondition(OrchestrationStatusQueryCondition condition)
+        private OrchestrationInstanceStatusQueryCondition ToAzureStorageCondition(OrchestrationStatusQueryCondition condition)


Let's make this method static since it doesn't reference this anywhere.

Suggested change

private OrchestrationInstanceStatusQueryCondition ToAzureStorageCondition(OrchestrationStatusQueryCondition condition)

private static OrchestrationInstanceStatusQueryCondition ToAzureStorageCondition(OrchestrationStatusQueryCondition condition)

Doing so actually creates a slight performance improvement but more importantly helps the reader better understand the scope of the method.

cgillum · 2022-04-12T04:40:27Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

        /// <summary>
        /// Return orchestration instances which matches the runtimeStatus.
        /// </summary>
-        public IEnumerable<OrchestrationStatus> RuntimeStatus { get; set; }
+        public ICollection<OrchestrationRuntimeStatus>? RuntimeStatus { get; set; }


We should be using OrchestrationStatus here.

Suggested change

public ICollection<OrchestrationRuntimeStatus>? RuntimeStatus { get; set; }

public ICollection<OrchestrationStatus>? RuntimeStatus { get; set; }

cgillum · 2022-04-12T04:47:02Z

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

+        public async Task<OrchestrationStatusQueryResult> GetOrchestrationStateWithFiltersAsync(OrchestrationStatusQueryCondition condition, CancellationToken cancellationToken)
+        {
+            OrchestrationInstanceStatusQueryCondition convertedCondition = this.ToAzureStorageCondition(condition);
+            var statusContext = await this.GetOrchestrationStateAsync(convertedCondition, condition.PageSize, condition.ContinuationToken, cancellationToken);


Let's avoid using var unless the type is already mentioned in the right-hand expression (which it's not in this case).

Suggested change

var statusContext = await this.GetOrchestrationStateAsync(convertedCondition, condition.PageSize, condition.ContinuationToken, cancellationToken);

IList<OrchestrationState> statusContext = await this.GetOrchestrationStateAsync(convertedCondition, condition.PageSize, condition.ContinuationToken, cancellationToken);

src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs

src/DurableTask.Core/Query/IOrchestrationServiceQueryClient.cs

cgillum · 2022-04-12T05:01:11Z

src/DurableTask.Core/Query/OrchestrationStatusQueryCondition.cs

+    /// <summary>
+    /// Query condition for searching the status of orchestration instances.
+    /// </summary>
+    public class OrchestrationStatusQueryCondition


Let's rename this to OrchestrationQuery. It's more similar to other Microsoft query APIs, like Azure Storage table query APIs.

src/DurableTask.Core/Query/OrchestrationStatusQueryResult.cs

src/DurableTask.Core/Query/OrchestrationQueryResult.cs

cgillum

Very close, just a few more small things.

src/DurableTask.Core/Query/OrchestrationQuery.cs

src/DurableTask.Core/Query/OrchestrationQueryResult.cs

cgillum

A few more small details...

src/DurableTask.Core/Query/OrchestrationQueryResult.cs

…n if orchestrationState is null

src/DurableTask.Core/Query/OrchestrationQueryResult.cs

cgillum

Looks great!

add multi instance query support logics

7a454d9

kaibocai requested a review from cgillum April 11, 2022 20:59

kaibocai commented Apr 11, 2022

View reviewed changes

cgillum requested changes Apr 11, 2022

View reviewed changes

have return type as OrchestrationStatusQueryResult

99caa9c

cgillum requested changes Apr 12, 2022

View reviewed changes

remove unnecessary files copied from durable extension

aa58a8d

kaibocai commented Apr 12, 2022

View reviewed changes

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Show resolved Hide resolved

cgillum reviewed Apr 13, 2022

View reviewed changes

src/DurableTask.Core/Query/OrchestrationQuery.cs Outdated Show resolved Hide resolved

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

made property in OrchestrationQueryResult read only

7c92b5e

cgillum reviewed Apr 13, 2022

View reviewed changes

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

fix parameter name - make ContinuationToken nullable - throw exceptio…

2127a00

…n if orchestrationState is null

cgillum reviewed Apr 13, 2022

View reviewed changes

src/DurableTask.Core/Query/OrchestrationQueryResult.cs Outdated Show resolved Hide resolved

make continuationToken null in constructor

3076aec

cgillum approved these changes Apr 13, 2022

View reviewed changes

kaibocai marked this pull request as ready for review April 13, 2022 18:13

kaibocai merged commit 3641e3f into main Apr 13, 2022

cgillum deleted the kaibocai/multi-instance-query-support branch April 13, 2022 21:14

This was referenced Apr 14, 2022

Support multi-instance query by implementing interface in new interface in DTFx microsoft/durabletask-mssql#71

Closed

Support multi-instance query by implementing interface in new interface in DTFx microsoft/durabletask-netherite#135

Open

cgillum mentioned this pull request Apr 18, 2022

Update NETFX TFM to 4.6.2 #707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add multi instance query support logics #702

add multi instance query support logics #702

kaibocai commented Apr 11, 2022 •

edited

kaibocai Apr 11, 2022 •

edited

cgillum Apr 11, 2022

cgillum left a comment

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum Apr 11, 2022

cgillum left a comment

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum Apr 12, 2022

cgillum left a comment

cgillum left a comment

cgillum left a comment

		@@ -0,0 +1,81 @@
		// Copyright (c) .NET Foundation. All rights reserved.
		// Licensed under the MIT License. See LICENSE in the project root for license information.

	/// <param name="cancellationToken">Cancellation token that can be used to cancel the status query operation.</param>
	/// <param name="cancellationToken">Cancellation token that can be used to cancel the query operation.</param>

	var transferredCondition = TransferToAzureStorageCondition(condition);
	OrchestrationInstanceStatusQueryCondition convertedCondition = TransferToAzureStorageCondition(condition);

	private OrchestrationInstanceStatusQueryCondition ToAzureStorageCondition(OrchestrationStatusQueryCondition condition)
	private static OrchestrationInstanceStatusQueryCondition ToAzureStorageCondition(OrchestrationStatusQueryCondition condition)

	public ICollection<OrchestrationRuntimeStatus>? RuntimeStatus { get; set; }
	public ICollection<OrchestrationStatus>? RuntimeStatus { get; set; }

	var statusContext = await this.GetOrchestrationStateAsync(convertedCondition, condition.PageSize, condition.ContinuationToken, cancellationToken);
	IList<OrchestrationState> statusContext = await this.GetOrchestrationStateAsync(convertedCondition, condition.PageSize, condition.ContinuationToken, cancellationToken);

add multi instance query support logics #702

add multi instance query support logics #702

Conversation

kaibocai commented Apr 11, 2022 • edited

kaibocai Apr 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

cgillum left a comment

Choose a reason for hiding this comment

kaibocai commented Apr 11, 2022 •

edited

kaibocai Apr 11, 2022 •

edited