Explicit taskHub parameter for stored procedures #167
Conversation
…ter to the settings. Breaking changes in vInstance and vHistory views usage.
|
@microsoft-github-policy-service agree |
|
@michaelplavnik thanks for submitting this PR. I enabled the CI and it looks like some tests are failing because of inserting
Can you take a look? |
…ame in TaskHubMode=1.
|
@cgillum Thanks for being patient :-) I have update this PR. Could you please run tests again, in my environment I cannot run performance tests. |
There was a problem hiding this comment.
Tests appear to be passing now. :)
I have a couple concerns about this PR from a breaking-change perspective:
- Adding
@TaskHubNameto all stored procedures - isn't this also a breaking change? Sprocs that don't start with_are considered public API and we need to maintain backwards compatibility.
- The modified
CurrentTaskHubsproc seems to no longer considerAPP_NAME(). Isn't this also technically a breaking change?
Is there any way to make these changes non-breaking? I'm less concerned about the changes to vInstances and vHistory.
|
@cgillum You are correct of cause that changes are breaking (and in case of vInstances, vHistory also hidden). Now, I know that you prefer to keep it backward compatible (vs. explicit breaking change). Let's see if I can make it backward compatible in all cases. |
…end of parameter list and make it optional. Restore presence of taskHubName in connection string by default. Call all functions with SP approach.
|
@cgillum Finally got some time to remove braking changes. However, functions now have private implementation and non breaking public wrapper. Please advise if you want public wrapper with explicit parameter as well. |
|
little bump on this @cgillum |
| /// </summary> | ||
| /// <seealso cref="SqlOrchestrationService.CreateAsync(bool)"/> | ||
| [JsonProperty("createMultiTennantTaskHub ")] | ||
| public bool CreateMultiTennantTaskHub { get; set; } = true; |
There was a problem hiding this comment.
According to Wikipedia, "multitenant" is a single word with just one "n". This is also the spelling we use in our documentation. Let's update all the new code in this PR to use that spelling.
Example here:
| public bool CreateMultiTennantTaskHub { get; set; } = true; | |
| public bool CreateMultitenantTaskHub { get; set; } = true; |
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="SqlOrchestrationServiceSettings"/> class. | ||
| /// </summary> | ||
| /// <param name="connectionString">The connection string for connecting to the database.</param> | ||
| /// <param name="taskHubName">Optional. The name of the task hub. If not specified, a default name will be used.</param> | ||
| /// <param name="schemaName">Optional. The name of the schema. If not specified, the default 'dt' value will be used.</param> | ||
| public SqlOrchestrationServiceSettings(string connectionString, string? taskHubName = null, string? schemaName = null) | ||
| /// <param name="connectionStringWithTaskHubName">Optional. When true task hub name is added to connection string. | ||
| /// Default value is <c>true</c> to match how connection pool behaved before introdcution of this parameter.</param> |
There was a problem hiding this comment.
| /// Default value is <c>true</c> to match how connection pool behaved before introdcution of this parameter.</param> | |
| /// Default value is <c>true</c> to match how connection pool behaved before introduction of this parameter.</param> |
| @@ -19,6 +19,7 @@ namespace DurableTask.SqlServer | |||
| using DurableTask.Core.Query; | |||
| using DurableTask.SqlServer.SqlTypes; | |||
| using DurableTask.SqlServer.Utils; | |||
| using Dynamitey.DynamicObjects; | |||
There was a problem hiding this comment.
Is this being used or can it be removed?
…elplavnik/durabletask-mssql into feature/explicit-hub-param
Resolve spelling issues raised by @cgillum in PR review.
|
Sorry for a long delay. I have merged latest changes for main. In addition, I have added ability to set connection string as environment variable for integration tests. |
Resolves issue #162.
Add TaskHubName parameter to all relevant SPs.
Add parameter to initialize TaskHubMode to the settings class.
NOTE: Breaking changes in vInstance and vHistory views usage in case of TaskHubMode=0.