Make the rest of the services have a similar raw/user interface structure.

Make the rest of the services have a similar raw/user interface structure.

Also implement missing functionality from taskqueue/memcache in the memory
implementation.

R=dnj@chromium.org, estaab@chromium.org, tandrii@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/gae/commit/357724b6ecfae313081072c5e6a4e3a1c71bce66

[dnj, 2015-08-03 22:37:26]

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
File impl/memory/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:77: var exp time.Duration
Maybe:
exp := i.Expiration()
if exp != 0 {
  exp = now.Add(exp)
}

Why are you storing the expiration time as a "time.Duration"? It seems like it
should be a "time.Time". The math here is basically encapsulating a time.Time in
a time.Duration, which seems weird to me.

Also, why are you setting the expiration if it's already set? Is this a reset?
I'd imagine that you should set the expiration if it == 0, not != 0?

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:88: return newItem.duplicate(true)
This seems like a weird way of doing this. Allocate an object, populate it,
duplicate it, discard it, and return the duplicate. Why not just:

val := i.Value()
valCopy := make([]byte, len(val))
copy(valCopy, val)

return &mcItem{
  ...
  value: valCopy,
  ...
}

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:98: m.items[i.Key()] = m.mkItemLocked(now, i)
Let "m.items" be allowed to be nil and test/init here.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
Should just "nil" it here.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:114: func (m *memcacheData) hasLocked(now time.Time, key
string) bool {
I read this as "has locked?". Maybe rename, "hasItemLocked" for consistency?

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 && ret.Expiration()
< time.Duration(now.UnixNano()) {
Yeah ret.Expiration() really should just be a time.Time. Use
"time.Time{}.IsZero()" to test for zero value.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:177: now := clock.Now(m.ctx)
Do this once outside of the lock.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:193: now := clock.Now(m.ctx)
Do this once outside of the lock.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:220: m.data.setItemLocked(clock.Now(m.ctx), itm)
Get "now" once, outside of the lock/loop.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:221: cb(nil)
Consider setting the items within the lock, but doing the callbacks outside of
the lock.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:232: val, err := m.data.retrieveLocked(clock.Now(m.ctx),
k)
Get "now" once, outside of the lock/loop.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:269: now := clock.Now(m.ctx)
Do this outside of the lock.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:296: m.data.setItemLocked(now,
m.NewItem(key).SetValue(newval))
This isn't safe to do with just an RLock.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go
File impl/memory/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go#ne...
impl/memory/taskqueue.go:110: cb(t.deleteLocked(task, queueName))
Consider calling "deleteLocked" in the loop, aggregating errors, then doing the
callbacks outside of the lock scope.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go#ne...
impl/memory/taskqueue.go:124: t.named[queueName] = map[string]*tq.Task{}
Consider making this a "taskQueueData" method: resetTaskQueueWithLock. It should
be present in "taskqueue_data.go", where the structure is actually implemented.

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go
File impl/prod/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go#newco...
impl/prod/memcache.go:98: if me, ok := err.(appengine.MultiError); ok {
It's guaranteed that "DeleteMulti" will return a MultiError? Even if it fully
succeeds?

If not, a scenario can exist where the callbacks aren't invoked.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface.go
File service/memcache/interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface....
service/memcache/interface.go:7: // Interface is the full interface to the
memcache service.
Document when MultiError instances will be returned.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface....
service/memcache/interface.go:39: AddMulti(items []Item) error
Exported methods need comments.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/raw_interf...
File service/memcache/raw_interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/memcache/raw_interf...
service/memcache/raw_interface.go:12: type RawInterface interface {
Exported methods need comments.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/types.go
File service/memcache/types.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/memcache/types.go#n...
service/memcache/types.go:31: Key() string
Exported methods need comments.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/interface.go
File service/taskqueue/interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/interface...
service/taskqueue/interface.go:39: Purge(queueName string) error
Exported methods need comments.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/raw_inter...
File service/taskqueue/raw_interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/raw_inter...
service/taskqueue/raw_interface.go:11: type RawStatsCB func(*Statistics, error)
Exported types need comments.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/raw_inter...
service/taskqueue/raw_interface.go:18: Purge(queueName string) error
Exported methods need comments.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue.go
File service/taskqueue/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue...
service/taskqueue/taskqueue.go:61: i := 0
IMO cleaner to allocate "ret" with zero length and len(queueNames) capacity,
then fill via "append".

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go
File service/taskqueue/types.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:102: ret := &Task{}
ret := *t
...
return &ret

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:105: if t.Header != nil {
len(t.Header) > 0

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:114: if t.Payload != nil {
len(t.Payload) > 0

[iannucci, 2015-08-04 01:21:21]

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
File impl/memory/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:77: var exp time.Duration
On 2015/08/03 22:37:25, dnj wrote:
> Maybe:
> exp := i.Expiration()
> if exp != 0 {
>   exp = now.Add(exp)
> }
> 
> Why are you storing the expiration time as a "time.Duration"? It seems like it
> should be a "time.Time". The math here is basically encapsulating a time.Time
in
> a time.Duration, which seems weird to me.

https://github.com/golang/appengine/blob/master/memcache/memcache.go#L80

> 
> Also, why are you setting the expiration if it's already set? Is this a reset?
> I'd imagine that you should set the expiration if it == 0, not != 0?

? I'm calculating the expiration based on the input expiration and the current
time, unless the expiration is 0, which is special.

I could change the internal representation I guess... This isn't new code, this
is just moved from before.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:88: return newItem.duplicate(true)
On 2015/08/03 22:37:25, dnj wrote:
> This seems like a weird way of doing this. Allocate an object, populate it,
> duplicate it, discard it, and return the duplicate. Why not just:
> 
> val := i.Value()
> valCopy := make([]byte, len(val))
> copy(valCopy, val)
> 
> return &mcItem{
>   ...
>   value: valCopy,
>   ...
> }

Done

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:98: m.items[i.Key()] = m.mkItemLocked(now, i)
On 2015/08/03 22:37:25, dnj wrote:
> Let "m.items" be allowed to be nil and test/init here.

Why?

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
On 2015/08/03 22:37:25, dnj wrote:
> Should just "nil" it here.

Why?

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:114: func (m *memcacheData) hasLocked(now time.Time, key
string) bool {
On 2015/08/03 22:37:25, dnj wrote:
> I read this as "has locked?". Maybe rename, "hasItemLocked" for consistency?

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 && ret.Expiration()
< time.Duration(now.UnixNano()) {
On 2015/08/03 22:37:24, dnj wrote:
> Yeah ret.Expiration() really should just be a time.Time. Use
> "time.Time{}.IsZero()" to test for zero value.

I could do that in another CL. It'll cause more incompatibility with the
production implementation though, so I'm not sure why I would want to do this?

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:177: now := clock.Now(m.ctx)
On 2015/08/03 22:37:25, dnj wrote:
> Do this once outside of the lock.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:193: now := clock.Now(m.ctx)
On 2015/08/03 22:37:25, dnj wrote:
> Do this once outside of the lock.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:220: m.data.setItemLocked(clock.Now(m.ctx), itm)
On 2015/08/03 22:37:25, dnj wrote:
> Get "now" once, outside of the lock/loop.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:221: cb(nil)
On 2015/08/03 22:37:25, dnj wrote:
> Consider setting the items within the lock, but doing the callbacks outside of
> the lock.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:232: val, err := m.data.retrieveLocked(clock.Now(m.ctx),
k)
On 2015/08/03 22:37:25, dnj wrote:
> Get "now" once, outside of the lock/loop.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:269: now := clock.Now(m.ctx)
On 2015/08/03 22:37:25, dnj wrote:
> Do this outside of the lock.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:296: m.data.setItemLocked(now,
m.NewItem(key).SetValue(newval))
On 2015/08/03 22:37:25, dnj wrote:
> This isn't safe to do with just an RLock.

oops, typo

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go
File impl/memory/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go#ne...
impl/memory/taskqueue.go:110: cb(t.deleteLocked(task, queueName))
On 2015/08/03 22:37:25, dnj wrote:
> Consider calling "deleteLocked" in the loop, aggregating errors, then doing
the
> callbacks outside of the lock scope.

yeah mumble. I ended up doing a thing for all of them where I get the lock
1/loop (so as not to apply false atomicity), and I also batch the calls. gross.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/taskqueue.go#ne...
impl/memory/taskqueue.go:124: t.named[queueName] = map[string]*tq.Task{}
On 2015/08/03 22:37:25, dnj wrote:
> Consider making this a "taskQueueData" method: resetTaskQueueWithLock. It
should
> be present in "taskqueue_data.go", where the structure is actually
implemented.

Done.

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go
File impl/prod/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go#newco...
impl/prod/memcache.go:98: if me, ok := err.(appengine.MultiError); ok {
On 2015/08/03 22:37:25, dnj wrote:
> It's guaranteed that "DeleteMulti" will return a MultiError? Even if it fully
> succeeds?
> 
> If not, a scenario can exist where the callbacks aren't invoked.

Completely correct. That's why this method exists.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface.go
File service/memcache/interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface....
service/memcache/interface.go:7: // Interface is the full interface to the
memcache service.
On 2015/08/03 22:37:25, dnj wrote:
> Document when MultiError instances will be returned.

Done.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface....
service/memcache/interface.go:39: AddMulti(items []Item) error
On 2015/08/03 22:37:25, dnj wrote:
> Exported methods need comments.

Not on an interface, and if the reader can't figure out that these are related
to the documented ones then they probably shouldn't be using the library.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/raw_inter...
File service/taskqueue/raw_interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/raw_inter...
service/taskqueue/raw_interface.go:11: type RawStatsCB func(*Statistics, error)
On 2015/08/03 22:37:25, dnj wrote:
> Exported types need comments.

Done.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go
File service/taskqueue/types.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:102: ret := &Task{}
On 2015/08/03 22:37:26, dnj wrote:
> ret := *t
> ...
> return &ret

Done.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:105: if t.Header != nil {
On 2015/08/03 22:37:25, dnj wrote:
> len(t.Header) > 0

Done.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/types.go#...
service/taskqueue/types.go:114: if t.Payload != nil {
On 2015/08/03 22:37:26, dnj wrote:
> len(t.Payload) > 0

Done.

[dnj (Google), 2015-08-04 03:48:03]

dnj@google.com changed reviewers:
+ dnj@google.com

[dnj (Google), 2015-08-04 03:48:03]

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
File impl/memory/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
On 2015/08/04 01:21:20, iannucci wrote:
> On 2015/08/03 22:37:25, dnj wrote:
> > Should just "nil" it here.
> 
> Why?

No reason to keep allocated empty maps around.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 && ret.Expiration()
< time.Duration(now.UnixNano()) {
On 2015/08/04 01:21:20, iannucci wrote:
> On 2015/08/03 22:37:24, dnj wrote:
> > Yeah ret.Expiration() really should just be a time.Time. Use
> > "time.Time{}.IsZero()" to test for zero value.
> 
> I could do that in another CL. It'll cause more incompatibility with the
> production implementation though, so I'm not sure why I would want to do this?

Storing nanosecond since epoch in a time.Duration is not an appropriate use of
the type. time.Duration is an unanchored time delta. While one can argue it's an
implicit delta from epoch, but since a specific point in time plus a
time.Duration is a time.Time, we might as well use that type :)

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go
File impl/prod/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/prod/memcache.go#newco...
impl/prod/memcache.go:98: if me, ok := err.(appengine.MultiError); ok {
On 2015/08/04 01:21:21, iannucci wrote:
> On 2015/08/03 22:37:25, dnj wrote:
> > It's guaranteed that "DeleteMulti" will return a MultiError? Even if it
fully
> > succeeds?
> > 
> > If not, a scenario can exist where the callbacks aren't invoked.
> 
> Completely correct. That's why this method exists.

Acknowledged.

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface.go
File service/memcache/interface.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/memcache/interface....
service/memcache/interface.go:39: AddMulti(items []Item) error
On 2015/08/04 01:21:21, iannucci wrote:
> On 2015/08/03 22:37:25, dnj wrote:
> > Exported methods need comments.
> 
> Not on an interface, and if the reader can't figure out that these are related
> to the documented ones then they probably shouldn't be using the library.

http://fxr.watson.org/fxr/ident?im=3;i=EDOOFUS

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue.go
File service/taskqueue/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue...
service/taskqueue/taskqueue.go:61: i := 0
On 2015/08/03 22:37:25, dnj wrote:
> IMO cleaner to allocate "ret" with zero length and len(queueNames) capacity,
> then fill via "append".

(Pang)

[iannucci, 2015-08-04 03:58:33]

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
File impl/memory/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
On 2015/08/04 03:48:03, dnj (Google) wrote:
> On 2015/08/04 01:21:20, iannucci wrote:
> > On 2015/08/03 22:37:25, dnj wrote:
> > > Should just "nil" it here.
> > 
> > Why?
> 
> No reason to keep allocated empty maps around.

yeah, but if it doesn't have data in it, then you're doing memcache wrong, and
allowing it to be nil moves initialization to less obvious places.

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 && ret.Expiration()
< time.Duration(now.UnixNano()) {
On 2015/08/04 03:48:03, dnj (Google) wrote:
> On 2015/08/04 01:21:20, iannucci wrote:
> > On 2015/08/03 22:37:24, dnj wrote:
> > > Yeah ret.Expiration() really should just be a time.Time. Use
> > > "time.Time{}.IsZero()" to test for zero value.
> > 
> > I could do that in another CL. It'll cause more incompatibility with the
> > production implementation though, so I'm not sure why I would want to do
this?
> 
> Storing nanosecond since epoch in a time.Duration is not an appropriate use of
> the type. time.Duration is an unanchored time delta. While one can argue it's
an
> implicit delta from epoch, but since a specific point in time plus a
> time.Duration is a time.Time, we might as well use that type :)

Yes, that's fine, but the sdk memcache API uses time.Duration. I don't want to
change the internal datatype in this CL, since it's big enough already, and I
don't want to explore all the places I'll have to patch up by changing this. So,
separate CL.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue.go
File service/taskqueue/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue...
service/taskqueue/taskqueue.go:61: i := 0
On 2015/08/04 03:48:03, dnj (Google) wrote:
> On 2015/08/03 22:37:25, dnj wrote:
> > IMO cleaner to allocate "ret" with zero length and len(queueNames) capacity,
> > then fill via "append".
> 
> (Pang)

How would this be cleaner? I still need i for the lme.

[iannucci, 2015-08-04 17:46:10]

On 2015/08/04 03:58:33, iannucci wrote:
> https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
> File impl/memory/memcache.go (right):
> 
>
https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
> impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/04 01:21:20, iannucci wrote:
> > > On 2015/08/03 22:37:25, dnj wrote:
> > > > Should just "nil" it here.
> > > 
> > > Why?
> > 
> > No reason to keep allocated empty maps around.
> 
> yeah, but if it doesn't have data in it, then you're doing memcache wrong, and
> allowing it to be nil moves initialization to less obvious places.
> 
>
https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
> impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 &&
ret.Expiration()
> < time.Duration(now.UnixNano()) {
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/04 01:21:20, iannucci wrote:
> > > On 2015/08/03 22:37:24, dnj wrote:
> > > > Yeah ret.Expiration() really should just be a time.Time. Use
> > > > "time.Time{}.IsZero()" to test for zero value.
> > > 
> > > I could do that in another CL. It'll cause more incompatibility with the
> > > production implementation though, so I'm not sure why I would want to do
> this?
> > 
> > Storing nanosecond since epoch in a time.Duration is not an appropriate use
of
> > the type. time.Duration is an unanchored time delta. While one can argue
it's
> an
> > implicit delta from epoch, but since a specific point in time plus a
> > time.Duration is a time.Time, we might as well use that type :)
> 
> Yes, that's fine, but the sdk memcache API uses time.Duration. I don't want to
> change the internal datatype in this CL, since it's big enough already, and I
> don't want to explore all the places I'll have to patch up by changing this.
So,
> separate CL.

Here's the separate CL: https://codereview.chromium.org/1262173003

> 
>
https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue.go
> File service/taskqueue/taskqueue.go (right):
> 
>
https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue...
> service/taskqueue/taskqueue.go:61: i := 0
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/03 22:37:25, dnj wrote:
> > > IMO cleaner to allocate "ret" with zero length and len(queueNames)
capacity,
> > > then fill via "append".
> > 
> > (Pang)
> 
> How would this be cleaner? I still need i for the lme.

[dnj, 2015-08-04 18:01:16]

lgtm

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go
File impl/memory/memcache.go (right):

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:111: m.items = map[string]*mcItem{}
On 2015/08/04 03:58:33, iannucci wrote:
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/04 01:21:20, iannucci wrote:
> > > On 2015/08/03 22:37:25, dnj wrote:
> > > > Should just "nil" it here.
> > > 
> > > Why?
> > 
> > No reason to keep allocated empty maps around.
> 
> yeah, but if it doesn't have data in it, then you're doing memcache wrong, and
> allowing it to be nil moves initialization to less obvious places. 

Makes sense :)

https://codereview.chromium.org/1270063003/diff/1/impl/memory/memcache.go#new...
impl/memory/memcache.go:116: if ok && ret.Expiration() != 0 && ret.Expiration()
< time.Duration(now.UnixNano()) {
On 2015/08/04 03:58:33, iannucci wrote:
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/04 01:21:20, iannucci wrote:
> > > On 2015/08/03 22:37:24, dnj wrote:
> > > > Yeah ret.Expiration() really should just be a time.Time. Use
> > > > "time.Time{}.IsZero()" to test for zero value.
> > > 
> > > I could do that in another CL. It'll cause more incompatibility with the
> > > production implementation though, so I'm not sure why I would want to do
> this?
> > 
> > Storing nanosecond since epoch in a time.Duration is not an appropriate use
of
> > the type. time.Duration is an unanchored time delta. While one can argue
it's
> an
> > implicit delta from epoch, but since a specific point in time plus a
> > time.Duration is a time.Time, we might as well use that type :)
> 
> Yes, that's fine, but the sdk memcache API uses time.Duration. I don't want to
> change the internal datatype in this CL, since it's big enough already, and I
> don't want to explore all the places I'll have to patch up by changing this.
So,
> separate CL.

Acknowledged.

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue.go
File service/taskqueue/taskqueue.go (right):

https://codereview.chromium.org/1270063003/diff/1/service/taskqueue/taskqueue...
service/taskqueue/taskqueue.go:61: i := 0
On 2015/08/04 03:58:33, iannucci wrote:
> On 2015/08/04 03:48:03, dnj (Google) wrote:
> > On 2015/08/03 22:37:25, dnj wrote:
> > > IMO cleaner to allocate "ret" with zero length and len(queueNames)
capacity,
> > > then fill via "append".
> > 
> > (Pang)
> 
> How would this be cleaner? I still need i for the lme.

Oh sorry, I didn't see the "i" being used there. Nevermind.

[iannucci, 2015-08-04 18:09:27]

Committed patchset #2 (id:20001) manually as
357724b6ecfae313081072c5e6a4e3a1c71bce66 (presubmit successful).