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

chore: refactor orchestrator and add more tests. #90

Merged
merged 4 commits into from
Dec 12, 2022
Merged

Conversation

thantos
Copy link
Contributor

@thantos thantos commented Dec 11, 2022

Closes #31

A start at refactoring the orchestrator.

  1. Load
  2. Execute
  3. Persist
  • Create a command executor which turns commands into events with client calls.
  • Added tests for the command executor
  • turns the core workflow execution into a generator
  • Save ALL new events to dynamo instead of just the ones generated by the workflow execution
  • Don't try to save events to dynamo from the activity worker
    • (worker -> sqs -> orchestrator -> dynamo)

return assertNever(command, `unknown command type`);
}

async function scheduleActivity(command: ScheduleActivityCommand) {
Copy link
Contributor

Choose a reason for hiding this comment

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

would it help the runtime performance to move these nested functions - scheduleActivity, scheduleChildWorkflow, etc out of the nesting, into the root of the class? So they're not reconstructed every executeCommand, which might happen multiple times over the lifetime of an orchestrator instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The number of invocations is fairly constrained and each invocation represents one or more network call that would dwarf any additional function bindings.

My thought is that the code cleanliness provided by re-use of the scoped variables trumps the cost of the creating the function on the 1-20 times this function is called per workflow execution run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok sure. I personally put nested functions in the negative-code-cleanliness category, but thats just subjective.

Copy link
Owner

@sam-goodwin sam-goodwin left a comment

Choose a reason for hiding this comment

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

Why is the executor cleaner than the orchestrator? Looks mostly the same

@thantos thantos merged commit 5bc0d4d into main Dec 12, 2022
@thantos thantos deleted the sussman/orch_events branch December 12, 2022 15:15
@thantos thantos mentioned this pull request Dec 21, 2022
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.

Optimize finishActivity
3 participants