Skip to content

Conversation

@sebastianburckhardt
Copy link
Collaborator

Previously the history event for creating suborchestration did not record the Tags dictionary. This creates challenges for the rewind implementation in the DTS backend, since it uses this history event to reconstruct the ExecutionStartedEvent in cases where a suborchestration was purged prior to a rewind.

Copy link
Contributor

Copilot AI left a 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 the Tags property to the SubOrchestrationInstanceCreatedEvent history event class to support rewind operations in the DTS backend. The Tags dictionary is needed to reconstruct ExecutionStartedEvent instances when sub-orchestrations have been purged before a rewind.

  • Added Tags property to SubOrchestrationInstanceCreatedEvent class with appropriate documentation and serialization attributes
  • Refactored tag merging in TaskOrchestrationDispatcher.cs to compute merged tags once and assign to both the history event and execution started event
  • Updated the abridged event generation to preserve the Tags property

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/DurableTask.Core/History/SubOrchestrationInstanceCreatedEvent.cs Added Tags property with DataMember attribute and updated copy constructor to include Tags
src/DurableTask.Core/TaskOrchestrationDispatcher.cs Refactored to compute mergedTags once and assign to both SubOrchestrationInstanceCreatedEvent and ExecutionStartedEvent
src/DurableTask.Core/OrchestrationRuntimeState.cs Updated abridged event generation to preserve Tags property

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sophiatev
Copy link
Contributor

This PR might be a good chance to add the warning that we discussed earlier: #1264

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@sebastianburckhardt
Copy link
Collaborator Author

This PR might be a good chance to add the warning that we discussed earlier: #1264

I think you can just merge the latter, I approved it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants