code review 5500057: dashboard: cache todo responses

dashboard: cache todo responses

[adg, 2011-12-21 05:45:51]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dsymonds, 2011-12-21 06:00:00]

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
File misc/dashboard/app/build/cache.go (right):

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:24: for _, key := range []string{uiCacheKey,
todoCacheKey} {
use memcache.DeleteMulti and save an RPC.

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:112: func unmarshalTodo(c appengine.Context, t
*memcache.Item) map[string]*Todo {
Why not use memcache.JSON.Foo?

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/hand...
File misc/dashboard/app/build/handler.go (right):

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/hand...
misc/dashboard/app/build/handler.go:154: c.Infof("cache hit")
probably Debugf

[adg1, 2011-12-21 06:11:36]

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
File misc/dashboard/app/build/cache.go (right):

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:24: for _, key := range []string{uiCacheKey,
todoCacheKey} {
On 2011/12/21 06:00:00, dsymonds wrote:
> use memcache.DeleteMulti and save an RPC.

Done.

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:112: func unmarshalTodo(c appengine.Context, t
*memcache.Item) map[string]*Todo {
On 2011/12/21 06:00:00, dsymonds wrote:
> Why not use memcache.JSON.Foo?

No CompareAndSwap, and I prefer to split the operations up so I can have more
granular logging and error handling.

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/hand...
File misc/dashboard/app/build/handler.go (right):

http://codereview.appspot.com/5500057/diff/3001/misc/dashboard/app/build/hand...
misc/dashboard/app/build/handler.go:154: c.Infof("cache hit")
On 2011/12/21 06:00:00, dsymonds wrote:
> probably Debugf

Done.

[dsymonds, 2011-12-21 06:21:14]

LGTM

http://codereview.appspot.com/5500057/diff/1005/misc/dashboard/app/build/cach...
File misc/dashboard/app/build/cache.go (right):

http://codereview.appspot.com/5500057/diff/1005/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:101: func todoCache(c appengine.Context) (item
*memcache.Item, miss bool) {
weird sense. I'd invert miss to ok.

[adg, 2011-12-21 06:24:48]

*** Submitted as 9a1f034fc127 ***

dashboard: cache todo responses

R=golang-dev, dsymonds, adg
CC=golang-dev
http://codereview.appspot.com/5500057

[adg1, 2011-12-21 06:28:45]

http://codereview.appspot.com/5500057/diff/1005/misc/dashboard/app/build/cach...
File misc/dashboard/app/build/cache.go (right):

http://codereview.appspot.com/5500057/diff/1005/misc/dashboard/app/build/cach...
misc/dashboard/app/build/cache.go:101: func todoCache(c appengine.Context) (item
*memcache.Item, miss bool) {
On 2011/12/21 06:21:15, dsymonds wrote:
> weird sense. I'd invert miss to ok.

It more clearly provides the info I'm interested in. "Was this a miss?" Note
that for the error state miss=false. It would be weird if the error state was
ok=true.

[rsc, 2011-12-21 06:46:14]

On Wed, Dec 21, 2011 at 01:28,  <adg@google.com> wrote:
> It more clearly provides the info I'm interested in. "Was this a miss?"

If you are interested in a negative result, you can use the ! operator.
I agree with David that returning ok bool is more consistent with
other functions and built-in operations.

Russ

[rsc, 2011-12-21 06:46:47]

On Wed, Dec 21, 2011 at 01:46, Russ Cox <rsc@golang.org> wrote:
> If you are interested in a negative result, you can use the ! operator.
> I agree with David that returning ok bool is more consistent with
> other functions and built-in operations.

For example if todoCache were a map...