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

Support Long Timers ⏳📈🚀🚀🚀 #340

Merged
merged 30 commits into from
Apr 14, 2022
Merged

Conversation

hossam-nasr
Copy link
Contributor

@hossam-nasr hossam-nasr commented Apr 9, 2022

Resolves #325. This PR leverages these changes in the extension: Azure/azure-functions-durable-extension#2134 to allow JS orchestrators to have timers longer than 6 days. Internally, when user code calls CreateTimer, if it is more than 6 days, a LongTimerTask, which is essentially a WhenAll task of several small sub-timers, is created instead of an atomic DFTimerTask.

Now, the JS SDK will become the first OOProc SDK to support long timers 🥳 This also unblocks #284

@hossam-nasr hossam-nasr changed the title Hossamnasr/long timers Support Long Timers ⏳📈🚀🚀🚀 Apr 9, 2022
@hossam-nasr hossam-nasr marked this pull request as ready for review April 9, 2022 03:43
@hossam-nasr hossam-nasr requested a review from davidmrdavid April 9, 2022 03:43
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.

Good job, I think this is close to merge. Left some comments. My main blocker at this point is that I would prefer not having the timer metadata be exposed in the orchestration context class :-)

src/durableorchestrationcontext.ts Outdated Show resolved Hide resolved
src/durableorchestrationcontext.ts Outdated Show resolved Hide resolved
src/task.ts Outdated Show resolved Hide resolved
@hossam-nasr hossam-nasr force-pushed the hossamnasr/long-timers branch from 9ccdef0 to 25e0a27 Compare April 11, 2022 21:58
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.

We're almost there!

src/durableorchestrationcontext.ts Outdated Show resolved Hide resolved
hossam-nasr and others added 3 commits April 11, 2022 16:02
Co-authored-by: David Justo <david.justo.1996@gmail.com>
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.

This looks pretty much ready to me, implementation-wise.

Just 2 more things:

  1. I realized last night this was missing a test-case. Can you please add the appropiate long timer test cases using the following segment of timer tests as an example?
    describe("createTimer()", () => {

For the above, you'll probably have to modify the TestHistories class to include a new history for long timers. That will be a little manual, but essentially you need to mock the expected History for a simple long timer scenario. My tip here is not to overcomplicate it, just mock the history for an orchestrator that schedules 1 long timer with, say, 2 sub-timers, and that's it.

As for how many long timer test cases you need, I'd say 2: one that tests that the orchestrator waits for the sub-timers, and another that tests that the orchestrator proceeds when all sub-timers finish.

  1. Can you do a manual check testing that long timers can be cancelled successfully? Now that timers can be arbitrarily long, it is important to make sure they can be cancelled at-will. The easiest way to test for cancellable timers it to utilize the human in the loop sample and have the activity in it always finish immediately. See the linked sample for more clarity. Just want to make sure this edge-case is at least manually tested as well. Thanks!

Copy link
Contributor Author

@hossam-nasr hossam-nasr left a comment

Choose a reason for hiding this comment

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

Added unit tests and also manually tested and verified that long timers get canceled :)

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.

The time has come ...
to merge this PR 🧙‍♂️

it looks good to me

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.

Support Long Timers
2 participants