Skip to content

Conversation

@YunchuWang
Copy link
Member

@YunchuWang YunchuWang commented May 7, 2025

This pull request introduces a new property, Tags, to the TaskScheduledEvent class in src/DurableTask.Core/History/TaskScheduledEvent.cs. This property is a dictionary designed to store key-value pairs of strings, providing a flexible way to include additional metadata.

Related prs:
https://github.com/microsoft/durabletask-protobuf/pull/43/files
microsoft/durabletask-dotnet#426
https://msazure.visualstudio.com/One/_git/AAPT-DTMB/pullrequest/12388270

Key Change:

  • Added a new Tags property to the TaskScheduledEvent class. This property is an IDictionary<string, string> with a default initialization to an empty dictionary, allowing for the storage of string-based key-value metadata.

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

It's a bit odd that we create this history event but don't expose any way for DTFx users to set it. I'm okay with this if we want to expose it via other libraries, but let's wait to merge it until we have a PR (in this repo or another repo) that actually uses the property in some end-to-end way.

@YunchuWang YunchuWang requested a review from cgillum May 12, 2025 05:44
@YunchuWang
Copy link
Member Author

hi @cgillum , i made more changes, can you take a look when have chance? thanks

Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

I'm worried that these overloads might be accidental breaking changes. Maybe we should revert those changes and only update the history events, which is what we did in the first version of your PR.

@YunchuWang YunchuWang requested a review from cgillum May 14, 2025 21:20
@YunchuWang YunchuWang requested a review from cgillum May 15, 2025 00:15
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

I haven't not read the new tests closely yet, but here is some feedback for now.

@YunchuWang YunchuWang requested a review from cgillum May 16, 2025 15:35
@YunchuWang
Copy link
Member Author

YunchuWang commented May 16, 2025

@cgillum hi Chris, comments addressed, all tests are passing, can you take another look? thanks

@YunchuWang YunchuWang merged commit 81aa432 into Azure:main May 16, 2025
30 of 44 checks passed
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.

2 participants