-
Notifications
You must be signed in to change notification settings - Fork 273
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
Add test for orchestrator termination #3023
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Glad to see we have these tests, and that they're exposing issues. I've added a few minor comments.
var outputs = new List<string>(); | ||
|
||
// Call our fake activity 100,000 times to simulate an orchestration that might run for >= 10,000s (2.7 hours) | ||
for (int i = 0; i < 100000; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that a large fan-out like this may make the test unstable on the low-powered CI machines due to the memory and I/O requirements. Do we need a fan-out at all or could we just use an orchestration that sleeps for a long time or waits for an external event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a fan-out though, the activities are called sequentially. I agree, it's overkill though since most of the tests run in well under a minute, I can shorten it, the idea behind writing the orchestrator this way was to demonstrate what might cause an orchestration with problematic runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, sorry, I misread the code. OK, I'm less worried about this then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just one small ask but otherwise I think this can be merged when you're ready.
Adds a test for orchestrator termination to the new E2E test suite
Pull request checklist
pending_docs.md
release_notes.md
/src/Worker.Extensions.DurableTask/AssemblyInfo.cs
dev
andmain
branches and will not be merged into thev2.x
branch.