-
Notifications
You must be signed in to change notification settings - Fork 64
Enable replay V2 #291
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
Enable replay V2 #291
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.
I'm glad to see progress here. I think it largely looks good, but I do have some questions, most of them likely due to my own unfamiliarity with the codebase.
if self.durable_context._replay_schema == ReplaySchema.V1: | ||
self.durable_context.actions.append(generation_state.actions) | ||
else: | ||
self.durable_context.actions[0].append(generation_state.actions) |
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.
Do we even generate TaskSet even more in V2?
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.
We do! It's just its corresponding actions
field that is now represented with either a WhenAllAction
or WhenAnyAction
.
I did originally think of removing the TaskSet
class with this PR, but that broke far too many unit tests to be viable at the moment. It is high on my priority list though
end_time = None | ||
|
||
if replay_schema == ReplaySchema.V2: | ||
actions = WhenAllAction(actions) |
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.
Isn't WhenAllAction
a single action, and actions expecting an Action[]
?
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.
You're asking why the single action WhenAllAction
is assigned to a variable named actions
, right? Yeah that was a weird choice on my end. Looking back, it's not very readable.
The reason is that, in the case of V1
replays, actions
is a list of action objects. Since we already use this variable in constructing the TaskSet below, I figured I would just override its contents for V2 and keep the remaining logic intact.
In general, a lot of this PR is designed to modify as little of the existing logic as possible, as I worried that too big of a refactor would risk introducing bugs. That being said, in this case we're talking about a specific variable name, so I don't mind just replacing the name with something like "action_payload" which works for V1 and V2.
task4 = context.task_any([task3]) | ||
task5 = context.task_all([task1, task2, task4]) | ||
task6 = context.task_any([task5]) | ||
yield task6 |
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 such a nested test case!
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.
Consider this approved with suggestions!
This PR is a follow-up to Azure/azure-functions-durable-extension#1836
It implements the V2 replay protocol on the Python SDK. The changes are almost exclusively with respect to the replay output, not the iteration through the history.
The change is primarily to represent compound tasks (WhenAll and WhenAny) as actions themselves, instead of simply appending their argument tasks to the actions array.
By default, we continue outputting the payload expected by the legacy (V1) replay schema.