LGTM with a few trivials. Thanks! https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go File state/api/client_test.go (right): https://codereview.appspot.com/85850043/diff/20001/state/api/client_test.go#newcode211 state/api/client_test.go:211: func echoUrl(c *gc.C) func(*websocket.Config) (io.ReadCloser, error) { On 2014/04/10 03:59:50, thumper wrote: > On 2014/04/09 10:28:40, rog wrote: > > Here's the source of your buffering problem. > > This fixes it: > > > > func echoUrl(c *gc.C) func(*websocket.Config) (io.ReadCloser, error) { > > message, err := json.Marshal(¶ms.ErrorResult{}) > > c.Assert(err, gc.IsNil) > > return func(config *websocket.Config) (io.ReadCloser, error) { > > pr, pw := io.Pipe() > > go func() { > > fmt.Fprintf(pw, "%s\n", message) > > fmt.Fprintf(pw, "%s\n", config.Location) > > }() > > return pr, nil > > } > > } > That is pretty nice. I take it io.Pipe also does the reading message boundaries? yup, io.Pipe does no buffering at all - each write corresponds exactly to each read. https://codereview.appspot.com/85850043/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode738 state/api/client.go:738: // Params for WatchDebugLog controls the filtering of the log messages. If the // DebugLogParams holds parameters for WatchDebugLog that // control the filtering of the log messages. ? Thanks a lot for documenting these, BTW. It makes a big difference. https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode747 state/api/client.go:747: // are set all modules are considered included. s/ / / Does the inclusion of a logging module here imply all its sub-modules too? https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode752 state/api/client.go:752: // ExcludeModule lists logging modules to exclude from the resposne. ditto https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode755 state/api/client.go:755: // have been sent, the socket is closed. // If zero, there is no limit. ? https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode763 state/api/client.go:763: // than the end. If replay is true, backlog is ignored. Nice. I started commenting on Backlog to say "but what if I want no lines of backlog?" - then I saw this. https://codereview.appspot.com/85850043/diff/60001/state/api/client.go#newcode821 state/api/client.go:821: logger.Debugf("initial line: %s", line) Just realised - we probably want %q here so that we don't get a blank line in the log. https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go File state/api/client_test.go (right): https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go#newcode140 state/api/client_test.go:140: return &badReader{ioutil.NopCloser(junk), err}, nil or ioutil.NopCloser(&badReader{err}) then you can lose the ReadCloser field inside badReader. https://codereview.appspot.com/85850043/diff/60001/state/api/client_test.go#newcode195 state/api/client_test.go:195: func echoUrl(c *gc.C) func(*websocket.Config) (io.ReadCloser, error) { s/echoUrl/echoURL/ (Go convention is to capitalise either all or none of an acronym) https://codereview.appspot.com/85850043/diff/60001/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/85850043/diff/60001/state/api/params/internal.go#newcode563 state/api/params/internal.go:563: // apiserver. s/apiserver/agent running the API server./ ? https://codereview.appspot.com/85850043/diff/60001/state/apiserver/client/client.go File state/apiserver/client/client.go (right): https://codereview.appspot.com/85850043/diff/60001/state/apiserver/client/client.go#newcode785 state/apiserver/client/client.go:785: // AgentVersion returns the current version that the api server is running. s/api/API/ https://codereview.appspot.com/85850043/