Code review comment for lp://staging/~dave-cheney/juju-core/069-worker-stater-I

great direction. some comments and queries below.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go
File worker/stater/stater.go (right):

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode27
worker/stater/stater.go:27: defaultMongoDataDir = "/var/lib/juju/db"
rather than having all these default constants and special methods to
return them, why not just define:

var DefaultConfig = Config{
     MongoDir: "/opt",
     MongoDataDir: "/var/lib/juju/db",
     MongoURL: mustParseURL("http://juju-dist.s3.amazonaws.com/tools"),
}

?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode31
worker/stater/stater.go:31: defaultPreallocSize = 1 << 20
s/defaultPreallocSize/preallocSize/

(it's not overridable, so "default" is a misnomer)

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode43
worker/stater/stater.go:43: // TODO(dfc) add support for watching the
state
i don't think we'll need to watch the state here.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode58
worker/stater/stater.go:58: func (sr *Stater) run() {
given the comment above, i'm not sure that structuring this as a worker
makes sense. the only thing that's continually running is mongod itself,
and that's managed by upstart.

i think perhaps we could just have two functions:

func Start(config *Config) error
func Stop(config *Config) error

each would be idempotent, so when the machiner is not running
JobServeState (or whatever that constant will be), it can call
stater.Stop to ensure that the state server is not running;
likewise it can call stater.Start to start it running (and maybe
download the mongo tar archive, install upstart script, etc).

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode63
worker/stater/stater.go:63: func (sr *Stater) Wait() error {
i think this can go, see above.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode69
worker/stater/stater.go:69: // the lifetime of the agent.
this isn't an exported function, so this comment seems inappropriate
here. the only callers are in this package.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode110
worker/stater/stater.go:110: log.Debugf("creating journal file: %v, size
%d", file, defaultPreallocSize)
i think we could probably do without the debug statements in this
function. creating files isn't that interesting. perhaps just one for
the function, if you think it's worth it.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode121
worker/stater/stater.go:121: log.Printf("fetching mongo from: %v",
sr.config.mongoUrl())
s/://

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode127
worker/stater/stater.go:127: return nil, fmt.Errorf("fetch %q return
status: %d %v", sr.config.mongoUrl(), resp.StatusCode, resp.Status)
"cannot fetch %q: %s", sr.config.mongoUrl, resp.Status)

(i believe the status string includes the status code).

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode133
worker/stater/stater.go:133: // archive, using the base directory of
sr.config.mongoDir().
// unpack reads a gzipped tar archive from r
// and unpacks it into sr.config.MongoDir.
?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode148
worker/stater/stater.go:148: return err
should we clean up the partially unpacked directory if this happens?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode170
worker/stater/stater.go:170: return fmt.Errorf("cannot handle tar file
with type: %d", hdr.Typeflag)
"cannot handle tar file %q with type %d", hdr.Name, hdr.Typeflag)

?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode189
worker/stater/stater.go:189: MongoUrl *url.URL
if we're only ever going to use it as a string (http.Get takes a
string), why not store it as a string?

also
s/Url/URL/

https://codereview.appspot.com/7134051/diff/20001/worker/stater/tools.go
File worker/stater/tools.go (right):

https://codereview.appspot.com/7134051/diff/20001/worker/stater/tools.go#newcode9
worker/stater/tools.go:9: func createPreallocFile(name string, size int)
error {
i'm not sure this function justifies its own file.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/tools.go#newcode18
worker/stater/tools.go:18: if _, err := f.Seek(int64(size)-1, 0); err !=
nil {
f.Truncate(int64(size)) ?

https://codereview.appspot.com/7134051/