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.

Sign up for GitHub

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

Jump to bottom

Upgrade Durable V3.x to use .NET 6 Framework only #2864

Merged
merged 20 commits into from
Jul 29, 2024
Merged

Upgrade Durable V3.x to use .NET 6 Framework only #2864

merged 20 commits into from
Jul 29, 2024

Conversation

nytian
Copy link
Contributor

@nytian nytian commented Jun 26, 2024 •
edited
Loading

Issue describing the changes in this PR

  • Update WebJobs.Extensions.DurableTask to use dotnet 6.0 only
  • Remove all #if !Functions v1 section
  • Remove all #if FUNCTIONS_V2_OR_GREATER, which is eqult to #if !Functions v1
  • Remove tests about Smoke Tests Functions v1, v2 and v3. And update tests to use Azure Functions Runtime v4.
  • Remove #if Functions_V2 or greater and #ifFunctions_V3 or greater since DF v3 will only support Functions v4.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

@nytian nytian changed the base branch from dev to v3.x June 26, 2024 04:50
nytian
nytian commented Jun 26, 2024
View reviewed changes
src/WebJobs.Extensions.DurableTask/Listener/DurableTaskTargetScaler.cs
var controlQueueMessages = controlQueueLengths.Sum();
var activeControlQueues = controlQueueLengths.Count(x => x > 0);
var controlQueueMessages = controlQueueLengths!.Sum();
var activeControlQueues = controlQueueLengths!.Count(x => x > 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only pass the build/release CI after I added this !. Otherwise, it will show me a error that controQueueLengths could be null. I am not sure why this is triggered now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose .NET6 might have better null-value detection / nullability analysis

@nytian
Copy link
Contributor Author

nytian commented Jun 26, 2024

The Functions v1 tests CI fail because V1 tests were deleted in this PR.

@nytian nytian requested review from jviau June 26, 2024 21:09
davidmrdavid
davidmrdavid reviewed Jun 26, 2024
View reviewed changes
Copy link
Contributor

@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.

Left some comments, but it looks correct to me. The change seems simple, just repeated many times over. I'll do another pass over the csproj tomorrow to confirm it looks right

src/WebJobs.Extensions.DurableTask/AzureStorageDurabilityProvider.cs Outdated Show resolved Hide resolved
src/WebJobs.Extensions.DurableTask/DurableTaskJobHostConfigurationExtensions.cs Outdated Show resolved Hide resolved
src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj Outdated Show resolved Hide resolved
test/FunctionsV2/WebJobs.Extensions.DurableTask.Tests.V2.csproj Show resolved Hide resolved
test/DFPerfScenarios/DFPerfScenariosV4.csproj
<TargetFramework>net60</TargetFramework>
<TargetFramework>net6.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

oh my. Was this even working before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we added this folder later and never put it in our build haha

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me just triple check then - is this now in the build? If so - where :)

src/WebJobs.Extensions.DurableTask/WebJobs.Extensions.DurableTask.csproj
@@ -46,90 +47,31 @@
<GeneratePackageOnBuild>true</GeneratePackageOnBuild>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't have time to look at this csproj today in detail, but at a glance looks good. Will do a side-by-side comparison tomorrow.

@davidmrdavid
Copy link
Contributor

@jviau would be good if you could skim over this. It LGTM for the most part, but would appreciate another set of eyes.

@jviau
Copy link
Contributor

jviau commented Jun 27, 2024

This looks good to me. Will need a lot of manual validation to ensure all scenarios still work.

@bachuv bachuv requested a review from davidmrdavid June 28, 2024 21:26
davidmrdavid
davidmrdavid requested changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@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 but before approving, we need to run the DF tests. For some reason, the GitHub action for this is not running here, probably because the PR is not against dev or main. So let's first get the GH action to run on all PRs, and run that here, before merging.

@davidmrdavid davidmrdavid mentioned this pull request Jul 2, 2024
Open
@nytian nytian merged commit f653d73 into v3.x Jul 29, 2024
4 checks passed
@nytian nytian deleted the nytian/dotnet-framework branch October 17, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidmrdavid davidmrdavid davidmrdavid approved these changes

@bachuv bachuv Awaiting requested review from bachuv

@cgillum cgillum Awaiting requested review from cgillum

@jviau jviau Awaiting requested review from jviau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nytian @davidmrdavid @jviau @bachuv