You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
HostConfigsmap[string]HostConfigByzantinemap[string]int// number of replicas to assign to each byzantine strategy// the host associated with each replica.hostsToReplicasmap[string][]hotstuff.ID// the host associated with each client.hostsToClientsmap[string][]hotstuff.IDreplicaOptsmap[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"
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.
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.
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 theorchestration
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 newcue-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 thevalidFor
slice could be a function.Experiment
struct stores too much information; it should only store static information provided via the command line or a configuration file. The results fromassignReplicasAndClients
and similar outputs should be kept separate from the input data. In theconfig.Config
struct in thecue-config
branch, I return aReplicaMap
defined like this:type ReplicaMap map[string][]*orchestrationpb.ReplicaOpts
.HostConfig
struct can be replaced by theconfig.Config
struct.Experiment
andHostConfig
containNumReplicas = Replicas
andNumClients = Clients
.internal-address
. See below.Experiment
should not be in the struct; there are too many maps with the same key.In
internal/cli/run.go
:func runController()
: This code is 119 lines long.The
internal-address
question; fromdocs/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):Do we need this internal address? @hanish520 @AlanRostem?
The text was updated successfully, but these errors were encountered: