-
Notifications
You must be signed in to change notification settings - Fork 67
Cue/cli compatibility fixes #169
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
Conversation
meling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return nil, err | ||
| } | ||
| opt.SetTreeOptions(e.hostCfg.BranchFactor, e.hostCfg.TreePositions, e.hostCfg.TreeDelta) | ||
| // opt.SetTreeOptions(e.hostCfg.BranchFactor, e.hostCfg.TreePositions, e.hostCfg.TreeDelta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| numReplicas := viper.GetInt("replicas") | ||
| numClients := viper.GetInt("clients") | ||
|
|
||
| // Kauri values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Tree-related values|
|
||
| cfgPath := viper.GetString("cue") | ||
|
|
||
| treeDelta := viper.GetDuration("tree-delta") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These belong together with tree values above (bf)
|
|
||
| var cfg *config.HostConfig | ||
| if cfgPath != "" { | ||
| cfg, err = config.Load(cfgPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Load was renamed in the other PR.
| replicaHosts := viper.GetStringSlice("replica-hosts") | ||
| clientHosts := viper.GetStringSlice("client-hosts") | ||
|
|
||
| var cfg *config.HostConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now ExperimentConfig
| } | ||
| if viper.GetBool("random-tree") { | ||
| rnd := rand.New(rand.NewSource(rand.Uint64())) | ||
| rnd.Shuffle(len(treePos), reflect.Swapper(treePos)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have tree.Shuffle() for this.
Previously, the
--hostscli flag was omitted when integrating Cue. Now I have introduced--client-hostsand--replica-hoststo the cli so that things can work like before and be compatible with Cue if the user chooses to.Upon implementing this fix, I see that the choice between Cue and Viper cli configuration can be improved so I opened issue #168