Sub-orchestrators now available #157
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ConnorMcMahon
left a comment
ConnorMcMahon
left a comment
There was a problem hiding this comment.
Looks good for the most part, but let's look to see if we can get some tests for this.
Collaborator
Author
|
Added a sample, and tests, and fixed errors. @ConnorMcMahon, when you get the chance, I'd appreciate another look :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses: #62
This PR enables sub-orchestrators and sub-orchestrators with retry options in Python Durable Functions 🥳 !
In implementing this, I had to review the structure of similar features, such as
call_activityandcall_activity_with_retryto see how those structured their implementation.What became very clear is that there are many opportunities for refactoring, because there is a lot of shared logic between a function like
call_activityandcall_sub_orchestrator. Also, there's a lot of shared logic between a function likefind_sub_orchestration_createdandfind_sub_orchestration_completed. To avoid deviating from the tried-and-tested approach, this PR only makes a first attempt at consolidating this shared logic: thefind_sub_orchestrator_created,find_sub_orchestration_completedandfind_sub_orchestration_failedare all abstracted into a core methodfind_sub_orchestratorwhich is able to handle all of their logic by simply including a specializingHistoryEventTypeas a new parameter.To show how this approach subsumes the logic of those three functions, I make their implementation be simply a call to
find_sub_orchestrationwith their appropriateHistoryEventTypeparameter. I believe this refactoring can be done for mostawaitableandyieldablefunctions in this SDK, and I hope we can do that refactor in a future PR.There are other opportunities for refactoring that I ignored, for time, but I think y'all will be able to spot them quickly.
I'm tracking the new refactoring task here: #167
One thing that is glaringly missing from this PR are tests for the sub-orchestrator. I can add them now, or add them in a future PR scheduled for the next release, especially considering that we are still in beta. Please let me know what you think, I don't mind either way.