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

Refactor: Simplify the experiment framework #150

Open
meling opened this issue Jan 6, 2025 · 4 comments
Open

Refactor: Simplify the experiment framework #150

meling opened this issue Jan 6, 2025 · 4 comments
Assignees
Labels
cleanup Improve readability, design patterns, and simplification

Comments

@meling
Copy link
Member

meling commented Jan 6, 2025

The current experiment framework in internal/orchestration has some methods that are rather complex and it is therefore difficult to adapt to new uses. I'm opening this to point out some concerns with the current code and propose a way to simplify the logic of parts of the orchestration package to make the code more maintainable.

In controller.go:

  • func (e *Experiment) assignReplicasAndClients() (err error): This code is 114 lines long and has a cyclomatic complexity of 16. Simplify by decoupling the logic into two separate methods, one for replicas and one for clients. Part of this code is already implemented in the new cue-config branch, which should be merged soon. It may not have the same behavior as before, but it is unclear that the current behavior is what we want.
  • func (e *Experiment) createReplicas() (cfg *orchestrationpb.ReplicaConfiguration, err error): This code is 60 lines long. This is not too bad, but it could still be simplified. For instance, the logic to compute the validFor slice could be a function.
  • The Experiment struct stores too much information; it should only store static information provided via the command line or a configuration file. The results from assignReplicasAndClients and similar outputs should be kept separate from the input data. In the config.Config struct in the cue-config branch, I return a ReplicaMap defined like this: type ReplicaMap map[string][]*orchestrationpb.ReplicaOpts.
  • The HostConfig struct can be replaced by the config.Config struct.
  • Both Experiment and HostConfig contain NumReplicas = Replicas and NumClients = Clients.
  • It is not clear to me if we need the internal-address. See below.
  • The following fields of Experiment should not be in the struct; there are too many maps with the same key.
HostConfigs map[string]HostConfig
Byzantine   map[string]int // number of replicas to assign to each byzantine strategy

// the host associated with each replica.
hostsToReplicas map[string][]hotstuff.ID
// the host associated with each client.
hostsToClients map[string][]hotstuff.ID
replicaOpts    map[hotstuff.ID]*orchestrationpb.ReplicaOpts

In internal/cli/run.go:

  • func runController(): This code is 119 lines long.

The internal-address question; from docs/experimentation.md:

Additionally, it is possible to specify an internal address for each host.
The internal address is used by replicas instead of the address used by the controller.
This is useful if the controller is connecting to the remote hosts using a global address,
whereas the hosts can communicate using local addresses.
The internal address is configured through the configuration file (loaded by the --config flag):

[[hosts-config]]
name = "hotstuff-worker-1"
internal-address = "192.168.10.2"

[[hosts-config]]
name = "hotstuff-worker-1"
internal-address = "192.168.10.3"

Do we need this internal address? @hanish520 @AlanRostem?

@meling meling added the cleanup Improve readability, design patterns, and simplification label Jan 6, 2025
@meling
Copy link
Member Author

meling commented Jan 6, 2025

We should remove this:

func parseByzantine() (map[string]int, error) {

We don't want to pass a map via the command line arguments; we now have support for passing this via the cue config file.

@meling
Copy link
Member Author

meling commented Jan 7, 2025

I checked @hanish520's private fork, and he removed the support for the internal address.

@AlanRostem
Copy link
Collaborator

Started work on this but I was thinking isn't it a better idea to make this compatible with the cue config right away? The replica and client assignment is done there anyway, so the old code which does that can be removed and we can try making everything compatible with cue-configs.

It's also possible to simplify first and open a different issue to make things compatible with cue in a later PR.

@meling
Copy link
Member Author

meling commented Jan 7, 2025

Yes, I think this makes a lot of sense.

Feel free to open separate issues for individual tasks or make check-list issues using the - [ ] markdown style from which you can create individual issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Improve readability, design patterns, and simplification
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

3 participants