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

Orchestrator, Executor and Executor Spawners #3

Merged
merged 32 commits into from
Dec 11, 2023
Merged

Orchestrator, Executor and Executor Spawners #3

merged 32 commits into from
Dec 11, 2023

Conversation

sifnoc
Copy link
Member

@sifnoc sifnoc commented Nov 24, 2023

Orchestrator can use ExecutorSpawner to manage Executors and the containers connected to Executors.

Here are the review requests:

  • Is it appropriate to use the term Spawner?
  • Is it unreasonable to add the function to manage services, meaning docker swarm containers, to Service Spawner?

The service spawner is not fully implemented yet. but the user can deploy mini-tree-generator as services with given docker-compose.yml file.

  • refactor helper functions: convert~

@sifnoc sifnoc marked this pull request as ready for review November 24, 2023 09:38
Copy link
Member

@enricobottazzi enricobottazzi left a comment

Choose a reason for hiding this comment

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

Before going deep into the code, I added some comments based on the first understanding of the README. I am a bit confused regarding the roles and functions of Orchestrator, Executor, Worker and the different Spawners. Maybe starting with a description of each role, their main functions and how they interact with each other in a whole flow might be beneficial.

Waiting for you to apply any required changes before continuing the review.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/executor/mod.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/executor/executor.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/executor/executor.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@alxkzmn alxkzmn self-requested a review November 28, 2023 07:12
@summa-dev summa-dev deleted a comment from sifnoc Nov 28, 2023
Copy link
Member

@enricobottazzi enricobottazzi left a comment

Choose a reason for hiding this comment

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

I getting closer to an understanding but I still have some issues. Maybe this is because my lack of experience in this area. At least this forces you to create a fool-proof codebase (:

In the README description I find there there's some mix between

  • data structures such as Orchestrator and Executor
  • external entities such as Worker which is not a data structure (therefore it shouldn't be described in code-style)
  • mention of mini-tree and mini-trees which again do not exist in our codebase as objects as I understood
  • description of how to use it with Swarm, which is something external to our library I guess
  • testing

In order to make a clearer understanding I propose to:

  • Clearly define what are the goals of summa-aggregation in pratical terms (i.e. scale merkle sum tree building)
  • Describe how to do that -> There's an Orchestrator who's role is to split the task of building a Merkle Sum Tree across many workers. Explain that workers are instantiated, in production, as cloud machines. There's no need to make reference to the code here or to code entities yet. Also a very elementary graphical illustration would help here
  • All the other comments related to the role of each component such as Spawner, AggregationMerkleSumTree, Orchestrator and Executor can be added directly to the code with in-line comments /// or to an example file.
  • Add the description on how to test the whole flow

As a last point it is still not clear to me the role of the mini_tree_server and mini_tree_generator in the whole context

README.md Outdated Show resolved Hide resolved
src/aggregation_merkle_sum_tree.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/orchestrator/orchestrator.rs Outdated Show resolved Hide resolved
src/orchestrator/orchestrator.rs Outdated Show resolved Hide resolved
src/orchestrator/orchestrator.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

I think it would be nice to link to this mini-readme from the main readme so that anyone who's deploying the project can easily find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Upated at e61a1da

Copy link

Choose a reason for hiding this comment

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

Seems like it got deleted somehow

- published: 4000
target: 4000
deploy:
replicas: 2
Copy link

Choose a reason for hiding this comment

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

Why 2 replicas, is this for local testing only?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the number of replicas as an example

Copy link

Choose a reason for hiding this comment

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

Is it actually used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, It is used when specified service_info to the CloudSpawner.

Copy link
Member Author

Choose a reason for hiding this comment

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

For your information, there is a load-balance layer between node and container in the ingress mode, which is a default network in Docker swarm.
image
https://docs.docker.com/engine/swarm/ingress/#publish-a-port-for-a-service

I will give you an example in summa-aggregation. Let’s assume two executors are spawned on the Orchestrator with replica parameter at 3. which means the request from the executors may be routed to another container.

This is good for liveness of workers, but I do not know how effective it will be to improve performance when there are more replicas than executor.

@alxkzmn alxkzmn self-requested a review December 7, 2023 07:23
Copy link

@alxkzmn alxkzmn left a comment

Choose a reason for hiding this comment

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

Requesting only the minor changes in the readme and comments, the rest looks fine to me.

Copy link
Member

@enricobottazzi enricobottazzi left a comment

Choose a reason for hiding this comment

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

The PR looks good and well-documented!

Just some small changes required here.

src/json_mst.rs Show resolved Hide resolved
src/aggregation_merkle_sum_tree.rs Outdated Show resolved Hide resolved
src/aggregation_merkle_sum_tree.rs Show resolved Hide resolved
src/orchestrator/mod.rs Show resolved Hide resolved
README.md Show resolved Hide resolved
src/orchestrator/csv/entry_16.csv Outdated Show resolved Hide resolved
bin/README.md Show resolved Hide resolved
examples/aggregation_flow.rs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@enricobottazzi
Copy link
Member

@sifnoc Can you also attach a comment to why we are returning an empty array in entries and leaves ?

@enricobottazzi
Copy link
Member

@sifnoc Can you also attach a comment to why we are returning an empty array in entries and leaves ?

I was thinking again about this. Can we simply return the actual entries and leaves ? Even if these are huge arrays, what would be the problem?

@sifnoc
Copy link
Member Author

sifnoc commented Dec 8, 2023

@sifnoc Can you also attach a comment to why we are returning an empty array in entries and leaves ?

I was thinking again about this. Can we simply return the actual entries and leaves ? Even if these are huge arrays, what would be the problem?

Firstly, we would need to add entries and leaves fields to the AggregationMerkleSumTree. Then, we'd have to copy the entries and leaves from each MerkleSumTree while initializing the AggregationMerkleSumTree. This is necessary because the entries() and leaves() methods have to return reference, like in entries() in Tree trait. However, this approach would also increase memory usage during the initialization of AggregationMerkleSumTree.
If you have any ideas or suggestions for this, I would be glad to hear them.

Copy link
Member

@enricobottazzi enricobottazzi left a comment

Choose a reason for hiding this comment

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

LGTM ∑☄️

@@ -137,7 +187,7 @@ impl ExecutorSpawner for LocalSpawner {
}
}

#[cfg(feature = "docker")]
// #[cfg(feature = "docker")]
Copy link

Choose a reason for hiding this comment

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

Can it be deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for point out, I will fix it upcoming last commit for #217 PR on summa-solvency.

@alxkzmn alxkzmn merged commit 7dba825 into main Dec 11, 2023
2 checks passed
@alxkzmn alxkzmn deleted the orchestrator branch December 11, 2023 07:36
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.

3 participants