-
Notifications
You must be signed in to change notification settings - Fork 284
Add Rewind to .NET isolated #3227
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
Conversation
| if (this.fixture.functionLanguageLocalizer.GetLanguageType() == LanguageType.DotnetIsolated) | ||
| { | ||
| Assert.Equal(HttpStatusCode.PreconditionFailed, rewindResponse.StatusCode); | ||
| } | ||
| else | ||
| { | ||
| Assert.Equal(HttpStatusCode.BadRequest, rewindResponse.StatusCode); | ||
| } |
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.
Why different response values depending on the language? Should we add comments explaining this?
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 tried to explain this in the comment for the first instance of where this happens (maybe the placement of the comment is not great)
// For all of the following tests, since Node throws a generic error in the case of a failure to rewind there is no great way
// to return specific status codes, whereas .NET isolated returns specific error types which can be used to return specific status codes.
// So, in the Node case, we simply check for the BadRequest status code.
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 is generally true for gRPC vs HTTP worker SDKs, and is also true for APIs like suspend/resume/terminate, where we have similar logic in the E2E tests.
| if (this.fixture.functionLanguageLocalizer.GetLanguageType() == LanguageType.DotnetIsolated) | ||
| { | ||
| Assert.Equal(HttpStatusCode.PreconditionFailed, rewindResponse.StatusCode); | ||
| } | ||
| else | ||
| { | ||
| Assert.Equal(HttpStatusCode.BadRequest, rewindResponse.StatusCode); | ||
| } |
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 is generally true for gRPC vs HTTP worker SDKs, and is also true for APIs like suspend/resume/terminate, where we have similar logic in the E2E tests.
This PR introduces the rewind API for .NET isolated apps. It also adds logic to correctly handle the new ExecutionRewoundEvent in the protos.
Note: I plan to add E2E tests for this once a new version of the DTS emulator is released with the backend implementation for rewind. In the meantime I have added thorough integration tests to the DTS repo. The only functionality not tested is if the API correctly handles trying to rewind un-failed or non-existent orchestrations, which I have tested manually and this works as expected.
Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.