Code review comment for lp:~jameinel/juju-core/gomaxprocs-apiserver-1290841

I like that much better, thanks.
Some further simplification suggestions below.

https://codereview.appspot.com/76120044/diff/20001/cmd/jujud/main.go
File cmd/jujud/main.go (right):

https://codereview.appspot.com/76120044/diff/20001/cmd/jujud/main.go#newcode1
cmd/jujud/main.go:1: // Copyright 2012-2014 Canonical Ltd.
I think it's reasonable to leave the copyright dates alone. That's
certainly the explicit convention in Go core, and Juju de facto.

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newcode75
cmd/jujud/machine.go:75: workersStarted chan struct{}
Perhaps some documentation for this might be appropriate, given
that it's only here for tests.

Or a method:

// WorkersStarted returns a channel that's closed when
// the top level workers have been started. This is
// provided for testing purposes.
func (a *MachineAgent) WorkersStarted() <-chan struct{}
     return a.workersStarted
}

https://codereview.appspot.com/76120044/diff/40001/cmd/jujud/machine.go#newcode345
cmd/jujud/machine.go:345: useMultipleCPUs()
If you took logging out of the utils package, you could add a log
message here instead:

    logger.Debugf("GOMAXPROCS set to %d", runtime.GOMAXPROCS(0))

That's probably sufficient for diagnosing gomaxprocs-related problems.

https://codereview.appspot.com/76120044/diff/40001/utils/export_test.go
File utils/export_test.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/export_test.go#newcode9
utils/export_test.go:9: func OverrideCPUFuncs(maxprocsfunc func(int)
int, numCPUFunc func() int) func() {
Assuming we go with this, I'd do:

var (
     GoMaxProcs = &gomaxprocs
     NumCPU = &numcpu
)

then we can use PatchValue in the tests.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go
File utils/gomaxprocs.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode11
utils/gomaxprocs.go:11: var gomaxprocs = runtime.GOMAXPROCS
This seems a bit of overkill for such a function with such simple logic.
I'd just test that it does the right thing (even though it's not a full
test on a machine with a single cpu). It'll fail on any machine
with more than one CPU, which seems adequate to me. Is that overly
pragmatic?

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode16
utils/gomaxprocs.go:16: // variable if it is set.
// UseMultipleCPUs sets GOMAXPROCS to the number of CPU
// cores unless overridden by the GOMAXPROCS environment
// variable.

?

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs.go#newcode20
utils/gomaxprocs.go:20: logger.Debugf("GOMAXPROCS already set in
environment to %q, %d internally",
I'm not that keen on utils functions producing log messages (and since
they're only at Debug level, they can't really be that important).
Honestly,
I'd just do:

func UseMultipleCPUs() {
      if os.Getenv("GOMAXPROCS") == "" {
          runtime.GOMAXPROCS(runtime.NumCPU())
      }
}

and be done with it. Alternatively, if it lived in cmd/jujud then
logging might be more appropriate, as it can be more location-specific
there.

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go
File utils/gomaxprocs_test.go (right):

https://codereview.appspot.com/76120044/diff/40001/utils/gomaxprocs_test.go#newcode28
utils/gomaxprocs_test.go:28: maxprocsfunc := func(n int) int {
or:

maxProcsFunc := func(n int) int {
     if n != 0 {
         s.setMaxProcs = n
     }
     return 1
}

then dispense with the channel and just
assert on s.setMaxProcs directly.

https://codereview.appspot.com/76120044/