The number of logical CPUs can be now set using --cpus flag #31
Conversation
lukher-dev
force-pushed
the
feat/CPUs-arg
branch
from
306d035 to
50e6506
Compare
There was a problem hiding this comment.
Thank you for taking the time to implement this feature!
I only have some minor comments on code formatting, flag description and config default value. Also, would you mind adding an appropriate change to README.md? For now, adding --cpus flag to help output will be sufficient, as the there is a documentation overhaul planned in #26 anyway.
integration_test.go
Outdated
| @@ -112,6 +114,7 @@ func TestGocannonDefaultValues(t *testing.T) { | |||
| cfg := common.Config{ | |||
| Duration: &duration, | |||
| Connections: &connections, | |||
| CPUs: &cpus, | |||
There was a problem hiding this comment.
There is a minor formatting issue here - tabs between : and & instead of spaces
integration_test.go
Outdated
| @@ -161,6 +165,7 @@ func TestGocanonWithPlugin(t *testing.T) { | |||
| cfg := common.Config{ | |||
| Duration: &duration, | |||
| Connections: &connections, | |||
| CPUs: &cpus, | |||
args.go
Outdated
| @@ -44,6 +46,9 @@ func parseArgs() (common.Config, error) { | |||
| PlaceHolder("file.csv"). | |||
| Short('o'). | |||
| String(), | |||
| CPUs: app.Flag("cpus", "Maximum number of logical CPUs"). | |||
There was a problem hiding this comment.
Descriptions of all other flags end with a full stop. One extra sentence could be added: Defaults to the number of available CPU threads (or the GOMAXPROCS env variable).
args.go
Outdated
| @@ -44,6 +46,9 @@ func parseArgs() (common.Config, error) { | |||
| PlaceHolder("file.csv"). | |||
| Short('o'). | |||
| String(), | |||
| CPUs: app.Flag("cpus", "Maximum number of logical CPUs"). | |||
| Default(strconv.Itoa(runtime.NumCPU())). | |||
There was a problem hiding this comment.
I'm a bit ify about setting the default value to runtime.NumCPU as it is possible that someone running Gocannon might set the desired value via GOMAXPROCS env variable. Consequently, such setting would be overwritten. Instead, I would suggest not setting the Default value at all, so that it is initialized with 0, which will not alter the existing value when passed to runtime.GOMAXPROCS(). Ditching the default flag value would require modifying printHeader() so that it retrieves the value from GOMAXPROCS() if the config value equals 0.
This pull request adds the feature mentioned in issue #20