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

[WIP] Re-add ScheduledStartTime to orchestration scheduling in .NET isolated #2805

Draft
wants to merge 5 commits into
base: v2.x
Choose a base branch
from
Draft

[WIP] Re-add ScheduledStartTime to orchestration scheduling in .NET isolated #2805

wants to merge 5 commits into from

Conversation

davidmrdavid
Copy link
Contributor

@davidmrdavid davidmrdavid commented Apr 30, 2024 •
edited
Loading

Follow up to:
#2634
#2544 (comment)

Fixes: microsoft/durabletask-dotnet#256

When refactoring the localGrpcListener (used by .NET isolated and Java) to use an IDurableClient (necessary for Distributed Tracing), we accidentally dropped the ability to set an orchestrator's "fireAt" time. This PR adds that back.

Since the IDurableClient does not expose functionality for scheduled orchestrators, this PR constructs the TaskHubClient, which does expose that functionality. It also leaves a few "TODO" comments to simplify the implementation in the future.
 
 Sitll pending:
 * Manual E2E test
 * Inclusion of distributed tracing regression test
 * Inclusion of test of scheduled orchestrations test.
 

Issue describing the changes in this PR

resolves microsoft/durabletask-dotnet#256

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.

@davidmrdavid davidmrdavid changed the title [WIP] Re-add ScheduledStartTime to orchestration schedling in .NET isolated [WIP] Re-add ScheduledStartTime to orchestration scheduling in .NET isolated Apr 30, 2024
davidmrdavid
davidmrdavid commented May 1, 2024
View reviewed changes
src/WebJobs.Extensions.DurableTask/LocalGrpcListener.cs
Comment on lines -235 to +255
InstanceIdStartsWith = query.InstanceIdStartsWith,
LastModifiedFrom = query.LastModifiedFrom?.ToDateTime(),
LastModifiedTo = query.LastModifiedTo?.ToDateTime(),
IncludeTransient = query.IncludeTransient,
IncludeState = query.IncludeState,
ContinuationToken = query.ContinuationToken,
PageSize = query.PageSize,
InstanceIdStartsWith = query.InstanceIdStartsWith,
LastModifiedFrom = query.LastModifiedFrom?.ToDateTime(),
LastModifiedTo = query.LastModifiedTo?.ToDateTime(),
IncludeTransient = query.IncludeTransient,
IncludeState = query.IncludeState,
ContinuationToken = query.ContinuationToken,
PageSize = query.PageSize,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ran a formatter and it fixed this.

jviau
jviau approved these changes May 2, 2024
View reviewed changes
Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if we should also use TaskHubClient in other locations in this file? Probably for another PR/time.

src/WebJobs.Extensions.DurableTask/LocalGrpcListener.cs
string instanceId = await this.GetClient(context).StartNewAsync(
request.Name, request.InstanceId, Raw(request.Input));
string instanceId = request.InstanceId ?? Guid.NewGuid().ToString("N");
TaskHubClient taskhubClient = new TaskHubClient(this.GetDurabilityProvider(context));
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to also provider ILoggerFactory to the ctor

src/WebJobs.Extensions.DurableTask/LocalGrpcListener.cs
TaskHubClient taskhubClient = new TaskHubClient(this.GetDurabilityProvider(context));

// TODO: Ideally, we'd have a single method in the taskhubClient that can handle both scheduled and non-scheduled starts.
// TODO: the type of `ScheduledStartTimestamp` is not nullable. Can we change it to `DateTime?` in the proto file?
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe proto uses its own type: Timestamp, which is an object, so it can be null (whereas DateTime and DateTimeOffset are structs). So no, we cannot change it to DateTime?.

src/WebJobs.Extensions.DurableTask/LocalGrpcListener.cs
string instanceId = request.InstanceId ?? Guid.NewGuid().ToString("N");
TaskHubClient taskhubClient = new TaskHubClient(this.GetDurabilityProvider(context));

// TODO: Ideally, we'd have a single method in the taskhubClient that can handle both scheduled and non-scheduled starts.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this was addressed in the durable-dotnet DurableTaskClient abstraction.

src/WebJobs.Extensions.DurableTask/LocalGrpcListener.cs Outdated
await taskhubClient.CreateOrchestrationInstanceAsync(request.Name, request.Version, instanceId, Raw(request.Input));
}

// TODO: should this not inclide the ExecutionId and other elements of the taskhubClient response?
Copy link
Contributor

Choose a reason for hiding this comment

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

We never included those in the proto. Something to consider for later.

@davidmrdavid davidmrdavid mentioned this pull request May 28, 2024
Merged
@tomseida tomseida mentioned this pull request Jul 29, 2024
Merged
@developmentvegas
Copy link

Hi, Is there any update on this? Several users are stuck trying to use the startAt and we can't ;(

@davidmrdavid davidmrdavid self-assigned this Sep 23, 2024
@davidmrdavid
Copy link
Contributor Author

This is missing some tests still unfortunately. Looking to get back to this soon, but have some urgent competing threads at the moment. If anyone is willing to contribute some tests in the meantime (described in the PR description), I'd be happy to incorporate them.

@cgillum cgillum mentioned this pull request Sep 24, 2024
Open
@tomseida
Copy link

tomseida commented Nov 1, 2024

We are approaching 11 months since the ticket was open presenting this defect. Any ETA when this will be available?

@nikosdelis
Copy link

Is this missing anything else now that @cgillum's comments have been resolved?

@nikosdelis
Copy link

nikosdelis commented Nov 22, 2024 •
edited
Loading

Thanks a million @cgillum and @davidmrdavid for bringing this closer to its completion. Any idea about when we can expect this to be shipped?

@tomseida
Copy link

tomseida commented Jan 9, 2025

Why has this pull request stalled? When can we expect these changes to be published? We are now at 13 months since this defect was opened.

@cgillum
Copy link
Member

cgillum commented Jan 9, 2025

Sorry for the delay. David has moved to a different team and @andystaples and I should be able to take over this PR shortly. We're currently setting up some new end-to-end test infrastructure to make sure these kinds of regressions don't happen again. Fingers crossed that we can have this done and merged in the next couple weeks. 🤞

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

@cgillum cgillum cgillum left review comments

@jviau jviau jviau approved these changes

Assignees

@davidmrdavid davidmrdavid

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ScheduleNewOrchestrationInstanceAsync() is Not Scheduling an Orchestration
6 participants
@davidmrdavid @developmentvegas @tomseida @nikosdelis @cgillum @jviau