Skip to content
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

Convert timer target to UTC #1138

Merged
merged 19 commits into from
Jul 23, 2024
Merged

Convert timer target to UTC #1138

merged 19 commits into from
Jul 23, 2024

Conversation

AnatoliB
Copy link
Collaborator

@AnatoliB AnatoliB commented Jul 16, 2024

Fixes #1127

The old Azure SDK (e.g. WindowsAzure.Storage v9.3.1) silently converts non-UTC datetime values to UTC before persisting them in a table. As a result, passing non-UTC datetime to CreateTimer worked, even though this is not a recommended pattern.

The new Azure SDK (e.g. Azure.Data.Tables v12.8.3) throws an exception on attempts to persist non-UTC datetime values, which causes a hard-to-diagnose issue later, after a successful CreateTimer invocation.

This fix makes sure CreateTimer always converts the provided datetime value to UTC, to match the end-to-end behavior after the Azure SDK upgrade.

Since this is a potentially breaking change to both DurableTask.Core and DurableTask.AzureStorage, it should be released only with DurableTask.Core v2 and DurableTask.AzureStorage v2.

2024-07-18 update: we decided to limit this fix to DurableTask.AzureStorage only, so no impact to DurableTask.Core

Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

First - congrats on the contribution!

Left some preliminary comments

src/DurableTask.Core/TaskOrchestrationContext.cs Outdated Show resolved Hide resolved
@AnatoliB AnatoliB changed the base branch from azure-storage-v12 to main July 18, 2024 17:42
@AnatoliB AnatoliB force-pushed the anatolib/fix-issue-1127 branch from 7437f94 to 1825acc Compare July 18, 2024 18:23
Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Running the CI on this branch shortly, let's wait for that to go green before merging. Unsure why it did not run automatically

Copy link
Collaborator

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

LGTM :)

@AnatoliB AnatoliB merged commit b01ef27 into main Jul 23, 2024
42 checks passed
@davidmrdavid davidmrdavid deleted the anatolib/fix-issue-1127 branch July 23, 2024 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants