-
Notifications
You must be signed in to change notification settings - Fork 320
Adding Missing Fields to Instance Entity and Fix Blob References #1272
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
Adding Missing Fields to Instance Entity and Fix Blob References #1272
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.
Pull request overview
This PR adds missing fields to the instance entity tracking in Azure Storage, addressing several gaps identified in a previous PR. The changes ensure that instance entities are properly populated even when they don't exist initially (e.g., for suborchestrations that complete in one execution), and fix handling of large termination outputs stored in blob storage.
Key changes:
- Refactored
UpdateStatusForTerminationAsyncto accept the fullExecutionTerminatedEventinstead of individual parameters, enabling proper handling of large outputs stored in blob storage - Added missing fields (Tags, Generation, TaskHubName) for suborchestration instance entities
- Ensured all instance entity fields are populated in
UpdateInstanceStatusAndDeleteOrphanedBlobsForCompletedOrchestrationAsyncfor cases where the entity was never created
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DurableTask.AzureStorage/Tracking/ITrackingStore.cs | Updated interface signature for UpdateStatusForTerminationAsync to accept ExecutionTerminatedEvent instead of separate parameters; fixed typo in documentation |
| src/DurableTask.AzureStorage/Tracking/TrackingStoreBase.cs | Updated abstract method signature to match interface changes |
| src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs | Implemented proper blob storage handling for termination output; added missing fields (TaskHubName, Tags, Generation) for suborchestrations; populated all fields in completion method |
| src/DurableTask.AzureStorage/Tracking/InstanceStoreBackedTrackingStore.cs | Updated implementation to use new method signature |
| src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs | Updated call site to pass full ExecutionTerminatedEvent object |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs
Outdated
Show resolved
Hide resolved
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs
Dismissed
Show dismissed
Hide dismissed
…vents for large input/outputs to extract the blob names
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs
Outdated
Show resolved
Hide resolved
cgillum
left a comment
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 fine, though I wonder if FirstOrDefault() would have been as good or a little better to avoid failures. But this is probably going to be an extreme corner case, so I'm not too worried about this.
Unfortunately I missed some things in this earlier PR. In particular, many fields of the instance entity were not being set in the case that no instance entity existed in the first place (and, in fact, I wasn't setting the output at all in any situation).
I also noticed I had other bugs. I actually should not be deleting the blob references in the call to synchronize the instance table with the history table. I misunderstood - the blobs should only be deleted in the case of a continue-as-new call when the history is overridden, which is never the case for an orchestration in a terminal state. I also updated the "input" and "output" fields of the instance entity to reference the blobs when appropriate. I also noticed that for the case of terminating a pending orchestration, the orchestration output was not set correctly if it was too large and needed to be stored in blob storage.
I also noticed some other places where fields were missing or incorrectly set (for example, for the case of a suborchestration, the tags, generation, and task hub name fields were not populated for the instance entity.
I made sure to add tests for synchronizing the instance and history tables for a failed and terminated orchestration (not just a completed one), and tested large inputs and outputs for all three of these cases. I also added tests for terminating a pending orchestration with a large output (termination reason).