group cache http thing

group cache http thing

[bradfitz, 2013-07-24 02:53:15]

LGTM https://codereview.appspot.com/11752043/diff/1/http.go File http.go (right): https://codereview.appspot.com/11752043/diff/1/http.go#newcode2 http.go:2: Copyright 2012 Google Inc. 2013 for this code https://codereview.appspot.com/11752043/diff/1/http.go#newcode38 http.go:38: Context ContextForHTTPRequest // may be nil put this comments before, as complete sentence, and describe the nil behavior. // Context optionally specifies a context for the server to use // when it receives an incoming request. // If nil, the server uses a nil Context. https://codereview.appspot.com/11752043/diff/1/http.go#newcode39 http.go:39: Transport TransportForContext // uses http.DefaultTransport if nil likewise, document more? also, are these types carrying their weight? they pollute the godoc of groupcache for little gain. I'd prefer to just move the types up here, and their docs. https://codereview.appspot.com/11752043/diff/1/http.go#newcode54 http.go:54: // ContextForHTTPRequest returns an a Context for the provided HTTP request. s/an a/a/ Say that this is for servers. And say that it may return nil, if no context is needed or found for the request? Optional. https://codereview.appspot.com/11752043/diff/1/http.go#newcode62 http.go:62: // eg "http://example.net:8000". e.g. https://codereview.appspot.com/11752043/diff/1/http.go#newcode74 http.go:74: // Each peer value should be a valid base URL, eg "http://example.net:8000". e.g. https://codereview.appspot.com/11752043/diff/1/http.go#newcode90 http.go:90: return httpGetter{p.Transport, peer + p.basePath}, true // TODO: pre-build a slice of *httpGetter whenever Set(peers ...string) is called to avoid these two allocations. https://codereview.appspot.com/11752043/diff/1/http.go#newcode148 http.go:148: func (h httpGetter) Get(context Context, in *pb.GetRequest, out *pb.GetResponse) error { I'd use a pointer receiver.

[adg, 2013-07-24 03:22:21]

I've added a test, which fails for some reason. I'm not sure why. I must be doing something wrong. Help? https://codereview.appspot.com/11752043/diff/1/http.go File http.go (right): https://codereview.appspot.com/11752043/diff/1/http.go#newcode2 http.go:2: Copyright 2012 Google Inc. On 2013/07/24 02:53:15, bradfitz wrote: > 2013 for this code Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode38 http.go:38: Context ContextForHTTPRequest // may be nil On 2013/07/24 02:53:15, bradfitz wrote: > put this comments before, as complete sentence, and describe the nil behavior. > > // Context optionally specifies a context for the server to use > // when it receives an incoming request. > // If nil, the server uses a nil Context. Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode39 http.go:39: Transport TransportForContext // uses http.DefaultTransport if nil On 2013/07/24 02:53:15, bradfitz wrote: > likewise, document more? > > also, are these types carrying their weight? they pollute the godoc of > groupcache for little gain. > > I'd prefer to just move the types up here, and their docs. Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode54 http.go:54: // ContextForHTTPRequest returns an a Context for the provided HTTP request. Deleted https://codereview.appspot.com/11752043/diff/1/http.go#newcode62 http.go:62: // eg "http://example.net:8000". On 2013/07/24 02:53:15, bradfitz wrote: > e.g. Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode74 http.go:74: // Each peer value should be a valid base URL, eg "http://example.net:8000". On 2013/07/24 02:53:15, bradfitz wrote: > e.g. Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode90 http.go:90: return httpGetter{p.Transport, peer + p.basePath}, true On 2013/07/24 02:53:15, bradfitz wrote: > // TODO: pre-build a slice of *httpGetter whenever Set(peers ...string) is > called to avoid these two allocations. Done. https://codereview.appspot.com/11752043/diff/1/http.go#newcode148 http.go:148: func (h httpGetter) Get(context Context, in *pb.GetRequest, out *pb.GetResponse) error { On 2013/07/24 02:53:15, bradfitz wrote: > I'd use a pointer receiver. Done.

[adg, 2013-07-24 03:38:25]

On 24 July 2013 13:22, <adg@golang.org> wrote: > I've added a test, which fails for some reason. I'm not sure why. I must > be doing something wrong. Help? > Ah, I see the problem. I need to run the caches nodes in separate processes. 

[adg, 2013-07-24 04:32:06]

I have a working test now! Whee!

[bradfitz, 2013-07-24 04:33:47]

Can't view it on my phone. Browsers and cookies suck. Will review after security line. On Jul 23, 2013 9:32 PM, <adg@golang.org> wrote: > I have a working test now! Whee! > > https://codereview.appspot.**com/11752043/<https://codereview.appspot.com/117... > 

[adg, 2013-07-24 04:35:18]

I made the CL public, take a look if bored in line https://codereview.appspot.**com/11752043/<https://codereview.appspot.com/117... 

[bradfitz, 2013-07-24 04:53:06]

LGTM https://codereview.appspot.com/11752043/diff/7002/http.go File http.go (right): https://codereview.appspot.com/11752043/diff/7002/http.go#newcode176 http.go:176: b, err := ioutil.ReadAll(res.Body) // TODO: avoid this garbage. https://codereview.appspot.com/11752043/diff/7002/http_test.go File http_test.go (right): https://codereview.appspot.com/11752043/diff/7002/http_test.go#newcode70 http_test.go:70: go awaitAddrReady(t, childAddr[i], &wg) defer cmd.Process.Kill() so if we add more tests later, the children are killed earlier? https://codereview.appspot.com/11752043/diff/7002/http_test.go#newcode128 http_test.go:128: func testChildAddr(t *testing.T) string { this is racy. at least comment that it is. the proper way would be to pass the l.File() as ExtraFiles to the child process, and then close your copy once the child starts. but whatever. this should usually work... just comment that you know it's partially flaky.

[adg, 2013-07-24 04:56:51]

https://codereview.appspot.com/11752043/diff/7002/http_test.go File http_test.go (right): https://codereview.appspot.com/11752043/diff/7002/http_test.go#newcode70 http_test.go:70: go awaitAddrReady(t, childAddr[i], &wg) On 2013/07/24 04:53:06, bradfitz wrote: > defer cmd.Process.Kill() > > so if we add more tests later, the children are killed earlier? I do that two lines from here https://codereview.appspot.com/11752043/diff/7002/http_test.go#newcode128 http_test.go:128: func testChildAddr(t *testing.T) string { On 2013/07/24 04:53:06, bradfitz wrote: > this is racy. at least comment that it is. > > the proper way would be to pass the l.File() as ExtraFiles to the child process, > and then close your copy once the child starts. > > but whatever. this should usually work... just comment that you know it's > partially flaky. Done.

[bradfitz, 2013-07-24 04:58:14]

https://codereview.appspot.com/11752043/diff/7002/http_test.go File http_test.go (right): https://codereview.appspot.com/11752043/diff/7002/http_test.go#newcode70 http_test.go:70: go awaitAddrReady(t, childAddr[i], &wg) On 2013/07/24 04:56:51, adg wrote: > On 2013/07/24 04:53:06, bradfitz wrote: > > defer cmd.Process.Kill() > > > > so if we add more tests later, the children are killed earlier? > > I do that two lines from here oh hah. I'm sleepy. but my way is 7x shorter.

[adg, 2013-07-24 04:59:39]

On 24 July 2013 14:58, <bradfitz@golang.org> wrote: > oh hah. I'm sleepy. but my way is 7x shorter. Oh well, it's committed now. 

[bradfitz, 2013-07-24 05:01:22]

Thanks! On Jul 23, 2013 9:59 PM, "Andrew Gerrand" <adg@golang.org> wrote: > > On 24 July 2013 14:58, <bradfitz@golang.org> wrote: > >> oh hah. I'm sleepy. but my way is 7x shorter. > > > Oh well, it's committed now. >