Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for orchestration “tags” in the SQL Server provider, including schema/storage, parameter plumbing, and validation via new unit/integration tests.
Changes:
- Persist tags in SQL by adding an
Instances.Tagscolumn and wiring stored procedures to read/write it. - Plumb tag serialization/deserialization into
SqlUtilsand pass tags intoCreateInstance/_CheckpointOrchestration. - Add unit + integration test coverage for tags (including backward-compat behavior when tags aren’t set).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DurableTask.SqlServer.Tests/Utils/TestService.cs | Adds a helper for starting orchestrations with tags in integration tests. |
| test/DurableTask.SqlServer.Tests/Unit/SqlUtilsTagTests.cs | New unit tests for AddTagsParameter serialization/null handling. |
| test/DurableTask.SqlServer.Tests/Integration/Orchestrations.cs | Adds integration tests validating tags behavior (set, empty, ContinueAsNew, etc.). |
| test/DurableTask.SqlServer.Tests/Integration/DatabaseManagement.cs | Updates expected schema script list to include schema-1.3.0.sql. |
| test/DurableTask.SqlServer.AzureFunctions.Tests/DurableTask.SqlServer.AzureFunctions.Tests.csproj | Changes test target framework to net8.0. |
| src/DurableTask.SqlServer/SqlUtils.cs | Adds tag read/write helpers and populates Tags on returned state/events. |
| src/DurableTask.SqlServer/SqlOrchestrationService.cs | Passes tags into relevant stored procedure calls. |
| src/DurableTask.SqlServer/Scripts/schema-1.3.0.sql | Adds Tags column to the Instances table. |
| src/DurableTask.SqlServer/Scripts/logic.sql | Extends sprocs/selects to accept/store/return tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...DurableTask.SqlServer.AzureFunctions.Tests/DurableTask.SqlServer.AzureFunctions.Tests.csproj
Outdated
Show resolved
Hide resolved
fanyirobin
force-pushed
the
dev/yifan1/tags
branch
from
3638da5 to
336173b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| string? json = tags != null && tags.Count > 0 | ||
| ? DTUtils.SerializeToJson(tags) | ||
| : null; | ||
| parameters.Add("@Tags", SqlDbType.VarChar, 8000).Value = (object?)json ?? DBNull.Value; |
There was a problem hiding this comment.
Given we have max length of 8000 characters, I think we need to validate the size of the tags payload to make sure it fits so that we don't run into data corruption issues. I suggest we do this at the time of orchestration creation so that we can fail early if the tags payload is too large.
| { | ||
| ordinal = reader.GetOrdinal("Tags"); | ||
| } | ||
| catch (IndexOutOfRangeException) |
There was a problem hiding this comment.
We don't catch this exception anywhere else that GetOrdinal is used. Is this necessary?
| 'Pending', | ||
| E.[TraceContext] | ||
| E.[TraceContext], | ||
| @Tags |
There was a problem hiding this comment.
I think there's an issue here where any tags explicitly created during sub-orchestration creation will get overwritten by the parent orchestration tags. Normally DTFx will try to merge any tags from the parent orchestration with any explicitly created sub-orchestration tags. However, it doesn't look like your implementation leverages the merged tags. I think what you'll need to do is get the tags from the entries in @NewOrchestrationEvents rather than passing in @Tags to this sproc to ensure we're providing the merged tags.
test cleanup rollback .net version change for tets
address comments Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…lServer.AzureFunctions.Tests.csproj address comments Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
address copilot comment. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
address comment Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
fanyirobin
force-pushed
the
dev/yifan1/tags
branch
from
6524a22 to
ef8b2ba
Compare
No description provided.