Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactored executor and executor manager #2186

Merged
merged 3 commits into from
Jul 21, 2023
Merged

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Jul 19, 2023

Refactored the executor and executor manager to be clone-able and more ergonomics.

Removed the use of executor manager with a vector of executor. Instead we favor composing executors into a new executor. Composing also allows the implementer to be precise about the execution orders. The old executor manager with its vector implementation is not explicit enough about the order when multiple executor is involved.

Re-implement the executor to be a function pointer instead of the Fn family traits. Traits will type erase and difficult to deal with when implementing clone.

Refactored the wasm related exectors to use the new scheme.

@yihuaf yihuaf self-assigned this Jul 19, 2023
@yihuaf yihuaf added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jul 19, 2023
Refactored the executor and executor manager to be clone-able and more
ergonomics.

Removed the use of executor manager with a vector of executor. Instead
we favor composing executors into a new executor. Composing also allows
the implementer to be precise about the execution orders. The old
executor manager with its vector implementation is not explicit enough
about the order when multiple executor is involved.

Re-implement the executor to be a function pointer instead of the Fn
family traits. Traits will type erase and difficult to deal with when
implementing clone.

Refactored the wasm related exectors to use the new scheme.

Signed-off-by: yihuaf <yihuaf@unkies.org>
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 19, 2023

I will be responsible for fixing any breakage with runwasi if required.

@yihuaf yihuaf requested review from utam0k, Furisto and a team July 19, 2023 19:31
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2023

Codecov Report

Merging #2186 (3595ea1) into main (6162618) will increase coverage by 0.06%.
The diff coverage is 11.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2186      +/-   ##
==========================================
+ Coverage   64.91%   64.97%   +0.06%     
==========================================
  Files         129      129              
  Lines       14985    14970      -15     
==========================================
  Hits         9727     9727              
+ Misses       5258     5243      -15     


impl Executor for DefaultExecutor {
fn exec(&self, spec: &Spec) -> Result<(), ExecutorError> {
pub fn get_executor() -> Executor {
Copy link
Member

Choose a reason for hiding this comment

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

May I ask you to add a comment to this function?

@utam0k
Copy link
Member

utam0k commented Jul 20, 2023

Can you add Developer Documentation on this so we can use another PR or just create an issue?
https://containers.github.io/youki/

yihuaf added 2 commits July 20, 2023 21:00
Signed-off-by: yihuaf <yihuaf@unkies.org>
Signed-off-by: yihuaf <yihuaf@unkies.org>
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jul 21, 2023

Can you add Developer Documentation on this so we can use another PR or just create an issue? https://containers.github.io/youki/

I added a section in the libcontainer.md. I don't think there is any previous mentions of executor. Let me know if this is good enough for now.

@yihuaf yihuaf requested a review from utam0k July 21, 2023 04:09
@utam0k
Copy link
Member

utam0k commented Jul 21, 2023

Can you add Developer Documentation on this so we can use another PR or just create an issue? https://containers.github.io/youki/

I added a section in the libcontainer.md. I don't think there is any previous mentions of executor. Let me know if this is good enough for now.

Can you post an example of how to use it, just another PR?

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thansks!

@utam0k utam0k merged commit c3559e4 into youki-dev:main Jul 21, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
breaking change kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants