-
Notifications
You must be signed in to change notification settings - Fork 71
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
Executor Refactor #286
Comments
There are a couple of things that would help me consider these changes:
|
I envision things being broken out as so:
TermsFor clarity I'll define some terms:
Example flowUltimately a full run through the execution pipeline might look something like this.
AdvantagesThis process provides us with a number of advantages. A few examples:
Implementation sequencing
ConcernsFor the most part I think this will be a much more flexible and maintainable solution. I am a bit unsure about the extra work we would be putting on the pipeline_coordinator though. It being a static named process, it could be a bit of a bottleneck. We could possible create another dynamic process called a post_processor? Once the executor returns the coordinator could spin up a post_processor process that is responsible for presentation and redirection. Thoughts? |
Summing up recent offline conversation between myself, @mpeck, and @christophermaier about how to approach restructuring the pipeline executor. High Level Summary
Why An Interpreter?
|
reference #282 for executor timeouts |
Reference #229 for anonymous users and safe commands |
Reference #208 for executor timings |
The Great Executor Refactor
The executor is in need of a refactor. Currently everything is pretty much crammed into a single module which makes it difficult to maintain. In order to improve maintainability and open up the possibility for future improvements we have some suggestions for a refactor.
Proposal - move templating out of the executor
Templating should be done independently of the executor. Specifically we are thinking about moving templating up to the initializer level. The initializer is a named process that starts executor processes. If we move templating up to this level, and perhaps rename the initializer to something more appropriate(pipeline_coordinator?), it will not only make things a bit cleaner, it gives us a place to more clearly implement things like error templating. It also makes it easier to eventually introduce parallel command execution and recursive alias expansion, because the executor can simple return json.
Proposal - move redirects out of the executor
In the same vein as the templating move. By moving redirects out of the we create a further separation of responsibilities for command execution.
Proposal - break the executor into two parts
The real meat of the executor exists in the
prepare_or_finish
function. Over time the function has grown and become difficult to follow. A major issue revolves around the fact that we iterate over multiple lists. At the top level we have a list of invocations, the pipeline. We iterate over the list of invocations executing each command in turn. Dependent on the type of command, they can be executed once or multiple times. And we further iterate over command input and output lists based on that execution type.We can simplify this by breaking the executor into two distinct components: The pipeline executor (iterates over the invocation list), and the command executor (managers how commands are executed). The pipeline executor's main purpose is to maintain the top level list of invocations, iterate over them and spin up a new command executor to run each invocation. The command executor is responsible for identifying the type of command to be executed and properly executing said command.
This would allow us to do some interesting things with command execution, namely things like parallel execution. It greatly simplifies the whole input output buffer bit. And it also allows us to do actual recursive expansion of aliases. Currently we just expand the alias and prepend the resulting pipeline to the invocation list. Using this method we can just spin up another pipeline executor as a child of the command executor. When the pipeline executor returns json, we just continue processing as normal. We can easily track how deep we go down the alias expansion rabbit hole and add configurable limits. Which should protect us from things like cyclic references.
I anticipate this issue spawning multiple additional issues. Also any discussion or feedback is welcome.
The text was updated successfully, but these errors were encountered: