Simple memory testing for gae/wrapper

Simple memory testing for gae/wrapper

Provides partial implementations of Memcache, TaskQueue and Datastore.

Most missing things I just didn't care to implement... Datastore queries are
conspicuously absent, and I'll be doing those in a separate CL.

R=maruel@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/f4e4857179f3e4f94f224cde281ae8e31643b921

[iannucci, 2015-05-24 18:15:20]

iannucci@chromium.org changed reviewers:
+ dnj@chromium.org

[iannucci, 2015-05-24 18:15:21]

PTAL, depends on https://codereview.chromium.org/1151473003/

[Vadim Sh., 2015-05-24 19:14:19]

On 2015/05/24 18:15:21, iannucci wrote:
> PTAL, depends on https://codereview.chromium.org/1151473003/

Stats (+4113 lines, -3 lines): http://i.imgur.com/DRSovrk.gif

[Vadim Sh., 2015-05-24 19:43:27]

Glanced over. Since it's for testing only, I do not care very much... If it
works, public API is reasonable and all crazy hack\complexity is hidden then
¯\_(ツ)_/¯

https://codereview.chromium.org/1152383003/diff/60001/go/Goopfile
File go/Goopfile (right):

https://codereview.chromium.org/1152383003/diff/60001/go/Goopfile#newcode9
go/Goopfile:9: github.com/luci/gkvlite #cf7fa95b097418138e80474e217d865e2eb853ac
you'd a mirror for this.. just a reminder

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/binutils.go:18: buf.WriteString(s)
writeBytes(buf, []byte(s))

to be symmetric with readString that uses readBytes.

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/binutils.go:35: val, err :=
funnybase.ReadUint(buf)
I don't fully understand yet where this function is used, but if it is used to
decode something coming from a wire, some simple size check wouldn't hurt, e.g

if val > 64 MB {
  return nil, "Are you mad?"
}

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:28: type memContext
[]memContextObj
memContext implements memContextObj too?

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:16: "appengine_internal"
noooooo!

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:17: pb
"appengine_internal/datastore"
noooooo! x2

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastoreData.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastoreData.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
datastore_data.go?

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastoreData.go:40: func (k *knrKeeper)
SetKindNameResolver(knr goon.KindNameResolver) {
looks like you didn't autoformat your code

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/gkvliteUtils.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/gkvliteUtils.go:16: // doesn't exist.
typo: forgot ")" :)

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:318: // NOTE: this is also a
slight divergence, but not a semantic one. IIUC, in
does datastore support "<=" index on bools? :)

[iannucci, 2015-05-24 20:04:57]

On 2015/05/24 at 19:14:19, vadimsh wrote:
> On 2015/05/24 18:15:21, iannucci wrote:
> > PTAL, depends on https://codereview.chromium.org/1151473003/
> 
> Stats (+4113 lines, -3 lines): http://i.imgur.com/DRSovrk.gif

Yeah I know :(

Be happy I split out the query stuff... it's going to be like 2k :(

[iannucci, 2015-05-24 20:33:55]

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/binutils.go:18: buf.WriteString(s)
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> writeBytes(buf, []byte(s))
> 
> to be symmetric with readString that uses readBytes.

I did this to avoid a copy (which I think is what []byte(string) does, right?)

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/binutils.go:35: val, err :=
funnybase.ReadUint(buf)
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> I don't fully understand yet where this function is used, but if it is used to
decode something coming from a wire, some simple size check wouldn't hurt, e.g
> 
> if val > 64 MB {
>   return nil, "Are you mad?"
> }

Good idea. It can actually be 2MB I think (since it's mostly for encoding []byte
in entities, which are limited to 1MB anyway)

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:28: type memContext
[]memContextObj
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> memContext implements memContextObj too?

yeah it does (plus some extra stuff)

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:17: pb
"appengine_internal/datastore"
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> noooooo! x2

Yeah I know... it's all for error types though...

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastoreData.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastoreData.go:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> datastore_data.go?

done

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastoreData.go:40: func (k *knrKeeper)
SetKindNameResolver(knr goon.KindNameResolver) {
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> looks like you didn't autoformat your code

I did, but gofmt will let methods stick together. I'll fix

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/gkvliteUtils.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/gkvliteUtils.go:16: // doesn't exist.
On 2015/05/24 at 19:43:26, Vadim Sh. wrote:
> typo: forgot ")" :)

done

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/globalinfo.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/globalinfo.go:41: func (gi *giImpl)
Namespace(ns string) (context.Context, error) {
I only implement Namespace for now because it interacts with datastore,
taskqueue and  memcache

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:318: // NOTE: this is also a
slight divergence, but not a semantic one. IIUC, in
On 2015/05/24 at 19:43:27, Vadim Sh. wrote:
> does datastore support "<=" index on bools? :)

I think it throws the filter out in the query optimizer. But that should still
work here anyway, since the equality prefix doesn't include inequality filters,
so the index scan will see them both.

But, yes, boolean values have a defined sort order, and you could do e.g. `>
false` (though you'd be a moron to do that). It's mostly important for Sort()
clauses, of which there may be multiple, and it could make sense to have
something sort by a boolean (getting all 'false's before all 'true's)

[M-A Ruel, 2015-05-25 18:21:11]

https://codereview.chromium.org/1152383003/diff/80001/go/Goopfile.lock
File go/Goopfile.lock (right):

https://codereview.chromium.org/1152383003/diff/80001/go/Goopfile.lock#newcode9
go/Goopfile.lock:9: github.com/luci/gkvlite
#cf7fa95b097418138e80474e217d865e2eb853ac
https://github.com/luci/gkvlite/blob/master/tools/view/view.go is an example of
what sucks with forking Go projects; the fact references are always absolute.

In the current case it doesn't matter _because_ it is a single-package project.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:44:
panic(fmt.Errorf("wrapper/memory: cannot get context item %q", item))
Why not return nil?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:70: newItm, err := itm.mkTxn(o)
You alias err here. I'd prefer to not use named return argument and instead
insert at the top of the function:

ret := make(memContext, 0, len(m))

it will reduce memory pressure in the success control flow.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:80: for i := range m {
Why happens if len(txnCtr) < len(m), panic. :)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:89: for i := range m {
same

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:134: txnMC, err :=
curMC.mkTxn(o)
You alias err here, so don't name the return variable.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:147: if err != nil {
if err = f(context.WithValue(d.c, memContextKey, txnMC)); err != nil {

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:151: return func() error {
I don't see why.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:36: ret =
&dsImpl{wrapper.DummyDS(), x, curGID(ic).namespace, ic}
return

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:38: ret =
&txnDsImpl{wrapper.DummyDS(), x, curGID(ic).namespace}
return

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:40: panic(fmt.Errorf("DS: bad
type: %v", dsd))
well, dsd will probably be nil so using ic would probably be better?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:43: return ret
Unnecessary, the compiler realizes all switch cases return or panic.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:24: lock sync.RWMutex
When the readers hold the lock for a trivial amount of time like here (line 38),
it's often more performant to use a normal sync.Mutex, due to its simplicity. In
particular, the chances to hit the fast path referenced below is extremely high,
offsetting the potential speed up by enabling concurrent access.

http://golang.org/src/sync/rwmutex.go

vs fast path of sync.Mutex.Lock
http://golang.org/src/sync/mutex.go

It's also much more easier to rationale about.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:56: sync.RWMutex
You are sure you want to expose RLock and friends?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:79: BrokenFeatures:
wrapper.NewBrokenFeatures(newDSError(pb.Error_INTERNAL_ERROR)),
By enabling support for dummy BrokenFeatures as explained on the other CL and
enabling per broken function an error, you'd remove this line entirely.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:104: err =
numData.UnmarshalBinary(v)
UnmarshalBinary mutates propertyList? Do not like. It's really at odd of the
general design of encoding packages in Go.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:161: func putPrelim(ns
string, knr goon.KindNameResolver, src interface{}) (key *datastore.Key, data
*propertyList, err error) {
I'd prefer not using named return values here for consistency with the rest.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:198: _, err =
plusPlusLocked(ents, groupMetaKey(key))
It's fine to group 198 & 199 on one line.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:228: return
datastore.ErrNoSuchEntity
That's weird, I would expect f() to do it.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:274: err =
oldPl.UnmarshalBinary(old)
Why do you unmarshal an entity when you are about to delete it?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:289: txn :=
obj.(*txnDataStoreData)
There should be a lock to ensure no mutation happens in the meantime.

The lock could either be global (slightly slower, easy to do) or per entity
(more complex, more concurrent)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:413: // will return an
error if we're crossing too many entity groups.
s/will return/Returns/
s/we're/the keys are/

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:447: key, err = func(key
*datastore.Key) (*datastore.Key, error) {
 func() would work, since it's in a closure.

personally I'd prefer a separate method.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:457: err =
td.writeMutation(false, key, plData)
45-458 fit one line

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:467: err :=
td.writeMutation(true, key, nil)
Fits one line

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:479: 
empty line not necessary

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore_goon.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_goon.go:29: // copied from goon
since it's internal and we need it for Put() :/
I'd prefer if it was in its own internal package, I don't like mixing third
party code and home grown code in the same repo.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:11: // boomStore is a
gkvlite.Store which will panic for anything which might
When I was reading the previous files, I found the name "boomStore" quite
opaque. kvStore or something with 'memory' would be preferable.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/globalinfo.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/globalinfo.go:30: return
&globalInfoData{namespace: ""}
return &globalInfoData{}

Then, why this function exists at all?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:9: 
group stdlib. seriously, I prefer goimports native formatting.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:76: ret = append(ret, &keyTok{
put on one line

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:84: func toksToKey(ns string, toks
[]*keyTok) *datastore.Key {
Since you use a named return value in the other method, you could use one here
too as well. You'd save a line.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:95: withNS nsOption = iota == 0
Too clever for no value, please use true and false.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:108: func keyFromByteString(nso
nsOption, d string) (*datastore.Key, error) {
I'd prefer you to only keep one of the 2 functions

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:129: var err error
Not needed

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:130: toks := []*keyTok{}
Move to line 143. Make it
toks := make([]*keyTok, 0, int(numToks))

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:139: numToks, err :=
funnybase.ReadUint(buf)
You should ensure numToks has a reasonable limit

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:155: tok.intID, err =
funnybase.ReadUint(buf)
You should ensure intID is positive, non zero.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:180: UserKeyOnly keyValidOption =
iota == 0
too clever again

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:233: testKey = newKey(ns, k.Kind(),
"", 1, k.Parent())
k = newKey(ns, k.Kind(), "", 1, k.Parent())

you don't need testKey

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/memcache.go:33: data    *memcacheData
Why a pointer?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/memcache.go:104: err := m.IsBroken()
fits one line

same below

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:56: err =
fmt.Errorf("propValTypeOf: unknown type of %v", v)
%#v

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:156: case pvBlobKey:
Do we need to implement this at all?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:197: if idx, ok = colIdx[p.Name];
!ok {
if idx, ok := colIdx[p.Name]; !ok {

and removing lines 195-196 would work just fine.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/schema (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/schema:1: index row encoding
This should go in README.md

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/taskqueue.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:167: mathRand *rand.Rand
Why not always create its own rand with Seet(0) ?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:236: for i := 0; i < 500; i++
{
Always 500 is excessive IMHO

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go:228: t.anony[queuename] =
[]*taskqueue.Task{}
you could set it to nil and it'd be fine

[iannucci, 2015-05-27 19:33:33]

PTAL, rebased + fixed lots

https://codereview.chromium.org/1152383003/diff/80001/go/Goopfile.lock
File go/Goopfile.lock (right):

https://codereview.chromium.org/1152383003/diff/80001/go/Goopfile.lock#newcode9
go/Goopfile.lock:9: github.com/luci/gkvlite
#cf7fa95b097418138e80474e217d865e2eb853ac
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> https://github.com/luci/gkvlite/blob/master/tools/view/view.go is an example
of what sucks with forking Go projects; the fact references are always absolute.
> 
> In the current case it doesn't matter _because_ it is a single-package
project.

Yeah, we're actually using the 32bitKeys branch here (where I fixed all the
imports: https://github.com/luci/gkvlite/blob/32bitKeys/tools/view/view.go)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:44:
panic(fmt.Errorf("wrapper/memory: cannot get context item %q", item))
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> Why not return nil?

Because it's meaningless. This object is only ever constructed with 2 objects in
it (for now, at some point later it will have 3), and those two objects go in
specific slots. If we can't find the item, it's an irrecoverable
program-terminating error. That's what panic is for.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:70: newItm, err := itm.mkTxn(o)
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> You alias err here. I'd prefer to not use named return argument and instead
insert at the top of the function:
> 
> ret := make(memContext, 0, len(m))
> 
> it will reduce memory pressure in the success control flow.

done, ditched append too since we don't really need it.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:80: for i := range m {
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> Why happens if len(txnCtr) < len(m), panic. :)

yes, that's intentional.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:89: for i := range m {
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> same

still intentional :)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:134: txnMC, err :=
curMC.mkTxn(o)
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> You alias err here, so don't name the return variable.

no I don't (http://play.golang.org/p/ZQcAvmI7WQ), but I also don't rely on the
named variable, so I removed it.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:147: if err != nil {
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> if err = f(context.WithValue(d.c, memContextKey, txnMC)); err != nil {

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/context.go:151: return func() error {
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> I don't see why.

used to have code after the function and needed to bound the lock. removed.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:36: ret =
&dsImpl{wrapper.DummyDS(), x, curGID(ic).namespace, ic}
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> return

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:38: ret =
&txnDsImpl{wrapper.DummyDS(), x, curGID(ic).namespace}
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> return

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:40: panic(fmt.Errorf("DS: bad
type: %v", dsd))
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> well, dsd will probably be nil so using ic would probably be better?

ic is also opaque :(. Added it anyway.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore.go:43: return ret
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> Unnecessary, the compiler realizes all switch cases return or panic.

yeah I was trying to assert that the various returned things implement the
interface above (Datastore+Testable), but there's better ways to enforce that
(like, the tests).

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:24: lock sync.RWMutex
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> When the readers hold the lock for a trivial amount of time like here (line
38), it's often more performant to use a normal sync.Mutex, due to its
simplicity. In particular, the chances to hit the fast path referenced below is
extremely high, offsetting the potential speed up by enabling concurrent access.
> 
> http://golang.org/src/sync/rwmutex.go
> 
> vs fast path of sync.Mutex.Lock
> http://golang.org/src/sync/mutex.go
> 
> It's also much more easier to rationale about.

sgtm, done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:28: func newKnrKeeper(fun
goon.KindNameResolver) *knrKeeper {
also removed, now embedding non-pointer

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:56: sync.RWMutex
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> You are sure you want to expose RLock and friends?

yeah maybe not. un-embedded.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:79: BrokenFeatures:
wrapper.NewBrokenFeatures(newDSError(pb.Error_INTERNAL_ERROR)),
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> By enabling support for dummy BrokenFeatures as explained on the other CL and
enabling per broken function an error, you'd remove this line entirely.

not entirely because the default error is still important.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:104: err =
numData.UnmarshalBinary(v)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> UnmarshalBinary mutates propertyList? Do not like. It's really at odd of the
general design of encoding packages in Go.

I'm emulating the appengine sdk's behavior here (e.g.
(*PropertyList).Load(chan<- Property) ). I agree that it sucks. If I fix it for
UnmarshalBinary, I am not going to be able to fix it for Load/Save, which need
to be self mutating to work with PropertyLoaderSaver. Is the discrepancy worth
it?

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:161: func putPrelim(ns
string, knr goon.KindNameResolver, src interface{}) (key *datastore.Key, data
*propertyList, err error) {
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> I'd prefer not using named return values here for consistency with the rest.

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:198: _, err =
plusPlusLocked(ents, groupMetaKey(key))
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> It's fine to group 198 & 199 on one line.

done, am glad I know this trick now.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:228: return
datastore.ErrNoSuchEntity
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> That's weird, I would expect f() to do it.

renamed f to make its function clearer

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:274: err =
oldPl.UnmarshalBinary(old)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> Why do you unmarshal an entity when you are about to delete it?

oh, that's a leftover from the next CL. We load the old one so that we can do a
differential update to the indices. I'll remove here.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:289: txn :=
obj.(*txnDataStoreData)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> There should be a lock to ensure no mutation happens in the meantime.
> 
> The lock could either be global (slightly slower, easy to do) or per entity
(more complex, more concurrent)

There is a lock, it's in RunInTransaction. It locks the transactionData and then
the main datastoreData for the duration of canApplyTxt + applyTxn.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:413: // will return an
error if we're crossing too many entity groups.
On 2015/05/25 at 18:21:08, M-A Ruel wrote:
> s/will return/Returns/
> s/we're/the keys are/

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:447: key, err = func(key
*datastore.Key) (*datastore.Key, error) {
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> func() would work, since it's in a closure.
> 

done

> personally I'd prefer a separate method.

I don't want it to be a separate method because I don't want anything else to
call it (unless you have a trick for that?)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:457: err =
td.writeMutation(false, key, plData)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> 45-458 fit one line

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:467: err :=
td.writeMutation(true, key, nil)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> Fits one line

done (I wish gofmt did this)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:479: 
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> empty line not necessary

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore_goon.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_goon.go:29: // copied from goon
since it's internal and we need it for Put() :/
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> I'd prefer if it was in its own internal package, I don't like mixing third
party code and home grown code in the same repo.

moved to internal/goon

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:11: // boomStore is a
gkvlite.Store which will panic for anything which might
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> When I was reading the previous files, I found the name "boomStore" quite
opaque. kvStore or something with 'memory' would be preferable.

memStore + memCollection

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/globalinfo.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/globalinfo.go:30: return
&globalInfoData{namespace: ""}
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> return &globalInfoData{}
> 
> Then, why this function exists at all?

because I dumbs

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:9: 
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> group stdlib. seriously, I prefer goimports native formatting.

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:76: ret = append(ret, &keyTok{
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> put on one line

find/replace fail. done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:84: func toksToKey(ns string, toks
[]*keyTok) *datastore.Key {
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Since you use a named return value in the other method, you could use one here
too as well. You'd save a line.

plus 4 characters! :P Done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:95: withNS nsOption = iota == 0
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Too clever for no value, please use true and false.

(technically, it's for TWO values.... :D)

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:108: func keyFromByteString(nso
nsOption, d string) (*datastore.Key, error) {
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> I'd prefer you to only keep one of the 2 functions

turns out I only used one.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:129: var err error
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Not needed

moved into the if block to make its use more clear.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:139: numToks, err :=
funnybase.ReadUint(buf)
On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> You should ensure numToks has a reasonable limit

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:155: tok.intID, err =
funnybase.ReadUint(buf)
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> You should ensure intID is positive, non zero.

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:180: UserKeyOnly keyValidOption =
iota == 0
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> too clever again

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/key.go:233: testKey = newKey(ns, k.Kind(),
"", 1, k.Parent())
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> k = newKey(ns, k.Kind(), "", 1, k.Parent())
> 
> you don't need testKey

yes I do. k could be `newKey(ns, k.Kind(), "foo", 10, nil)`.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/memcache.go:33: data    *memcacheData
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Why a pointer?

because I want multiple goroutines to see + manipulate the same memcache data

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/memcache.go:104: err := m.IsBroken()
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> fits one line
> 
> same below

done (everywhere)

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:56: err =
fmt.Errorf("propValTypeOf: unknown type of %v", v)
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> %#v

done

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:156: case pvBlobKey:
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Do we need to implement this at all?

maybe not... should I just panic for BlobKey's? I'm tempted to do that for
*Key's too... that would maybe let me decouple from "appengine/datastore".
Followup CL? Added TODO's

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:197: if idx, ok = colIdx[p.Name];
!ok {
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> if idx, ok := colIdx[p.Name]; !ok {
> 
> and removing lines 195-196 would work just fine.

yay!

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/schema (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/schema:1: index row encoding
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> This should go in README.md

done, added more words too

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/taskqueue.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:167: mathRand *rand.Rand
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Why not always create its own rand with Seet(0) ?

So that the test writer can control it

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:236: for i := 0; i < 500; i++
{
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> Always 500 is excessive IMHO

That's the actual name limit.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go:228: t.anony[queuename] =
[]*taskqueue.Task{}
On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> you could set it to nil and it'd be fine

done

[M-A Ruel, 2015-05-27 20:14:48]

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:79: BrokenFeatures:
wrapper.NewBrokenFeatures(newDSError(pb.Error_INTERNAL_ERROR)),
On 2015/05/27 19:33:31, iannucci wrote:
> On 2015/05/25 at 18:21:09, M-A Ruel wrote:
> > By enabling support for dummy BrokenFeatures as explained on the other CL
and
> enabling per broken function an error, you'd remove this line entirely.
> 
> not entirely because the default error is still important.

Yes but you don't need a function for that, simply doing:

BrokenFeatures: BrokenFeature{DefaultError: newDSError(...)},

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:104: err =
numData.UnmarshalBinary(v)
> Is the discrepancy worth it?

No.

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/80001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memory/plist.go:156: case pvBlobKey:
On 2015/05/27 19:33:32, iannucci wrote:
> On 2015/05/25 at 18:21:10, M-A Ruel wrote:
> > Do we need to implement this at all?
> 
> maybe not... should I just panic for BlobKey's? I'm tempted to do that for
> *Key's too... that would maybe let me decouple from "appengine/datastore".
> Followup CL? Added TODO's

I don't mind much.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg_test.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg_test.go:18: func TestGetEntityGroupVersion(t
*testing.T) {
Why none of your tests are t.Parallel()?

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg_test.go:23: type A struct {
a := &struct {
...
}{Val:10}

would work fine. You do not need to export the structure itself, just the
members.

In fact, you don't even need the named variable a but inlining it at the
ds.Put() call that would make the code a bit opaque.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:36: * nil, true, false
nil is not a type.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:83: ```
Why you indent above bot use ``` here?

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/binutils.go:16: func writeString(buf
*bytes.Buffer, s string) {
Why not a io.Writer? I'd be much more idiomatic.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:9: "infra/gae/libs/wrapper"
goimports really generated that? That would be highly surprising.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:42: if i, ok :=
memContextIndices[item]; !ok {
if i, ok := memContextIndices[item]; ok {
  return m[i]
}
panic(fmt.Errorf("wrapper/memory: cannot get context item %q", item))

another line saved.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:121: // a couple ways, for
example.
FWIU, it's really more that TaskQueue implicitly depends on DS since it uses the
datastore underneath.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:117: func
plusPlusLocked(ents *memCollection, key []byte) (int64, error) {
s/plusPlus/increment/g

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing
(right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing:4: 
?

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:18: basepb "appengine_internal/base"
why?

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go:31:
*wrapper.BrokenFeatures
Doesn't need to be a pointer.

[iannucci, 2015-05-27 21:36:22]

ptal

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg_test.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg_test.go:18: func TestGetEntityGroupVersion(t
*testing.T) {
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> Why none of your tests are t.Parallel()?

because the entire test suite takes .1 seconds and I wasn't worrying about it?

changed them all to Parallel. Now they take .1 seconds instead of .1 seconds.

\o/

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg_test.go:23: type A struct {
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> a := &struct {
> ...
> }{Val:10}
> 
> would work fine. You do not need to export the structure itself, just the
members.
> 
> In fact, you don't even need the named variable a but inlining it at the
ds.Put() call that would make the code a bit opaque.

Put uses reflection to discover the type name. An anonymous type wouldn't work
(though I could add an explicit 'kind' field to skip the reflection).

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:36: * nil, true, false
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> nil is not a type.

it is in datastore :)

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:83: ```
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> Why you indent above bot use ``` here?

done

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/binutils.go:16: func writeString(buf
*bytes.Buffer, s string) {
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> Why not a io.Writer? I'd be much more idiomatic.

because io.Writer.Write returns an error... since I only use this with
bytes.Buffer (whose Write method is guaranteed to only return nil as the error),
I didn't want to deal with the extra error checks.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:9: "infra/gae/libs/wrapper"
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> goimports really generated that? That would be highly surprising.

yeah it really did :(. I think it's because it thinks our infra package is part
of the stdlib. derp.

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:42: if i, ok :=
memContextIndices[item]; !ok {
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> if i, ok := memContextIndices[item]; ok {
>   return m[i]
> }
> panic(fmt.Errorf("wrapper/memory: cannot get context item %q", item))
> 
> another line saved.

done

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:121: // a couple ways, for
example.
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> FWIU, it's really more that TaskQueue implicitly depends on DS since it uses
the datastore underneath.

it also isn't allowed to have a Name parameter in the task. Also it's reliance
on the datastore is really to just attach an extra action (not really do a
datastore write). It's complicated :D

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:117: func
plusPlusLocked(ents *memCollection, key []byte) (int64, error) {
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> s/plusPlus/increment/g

done

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing
(right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing:4: 
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> ?

? indeed

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:18: basepb "appengine_internal/base"
On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> why?

So I can emulate the GetNamespace context.Call invocation. It's the only way to
explicitly set the Namespace field of the Key (without using unsafe)

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/taskqueue_data.go:31:
*wrapper.BrokenFeatures
On 2015/05/27 at 20:14:48, M-A Ruel wrote:
> Doesn't need to be a pointer.

done

[iannucci, 2015-05-28 20:55:21]

*hopefully-not-annoying ping* :D

[M-A Ruel, 2015-05-28 22:42:39]

https://chromiumcodereview.appspot.com/1152383003/diff/220001/go/src/infra/ga...
File go/src/infra/gae/libs/meta/eg_test.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/220001/go/src/infra/ga...
go/src/infra/gae/libs/meta/eg_test.go:18: func TestGetEntityGroupVersion(t
*testing.T) {
On 2015/05/27 21:36:22, iannucci wrote:
> On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> > Why none of your tests are t.Parallel()?
> 
> because the entire test suite takes .1 seconds and I wasn't worrying about it?
> 
> changed them all to Parallel. Now they take .1 seconds instead of .1 seconds.
> 
> \o/

There's two reasons;
- With Convey(), you make large test cases.
- You need to set GOMAXPROCS manually in an init() function.

It's more a statement to make it clear that there is no global context.

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:58: /* collections:
FTR, I prefer to use // all the time so if I need to comment out a block of
code, it's faster with /* */. Optional nit.

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:131: num++
Remove both num++, insert before line 133:
num++

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:198: defer d.Unlock()
Stay symmetrical between the calls.

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:232: err =
pl.UnmarshalBinary(pdata)
merge lines 232 et 233.

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:241: d.rwlock.RLock()
There will be enough concurrent calls to warrant a RWMutex?

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing
(right):

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing:4: 
?

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/memcache.go:29: *wrapper.BrokenFeatures
Why a pointer?

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/memcache.go:64: m.data.casID++
Not thread safe.

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/taskqueue.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:38: wrapper.DummyTQ(),
you create a TQ everytime?

https://chromiumcodereview.appspot.com/1152383003/diff/240001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:136: 
Isn't a lock missing here?

[iannucci, 2015-05-28 23:00:35]

thanks! ptal again :)

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg_test.go (right):

https://codereview.chromium.org/1152383003/diff/220001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg_test.go:18: func TestGetEntityGroupVersion(t
*testing.T) {
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> On 2015/05/27 21:36:22, iannucci wrote:
> > On 2015/05/27 at 20:14:47, M-A Ruel wrote:
> > > Why none of your tests are t.Parallel()?
> > 
> > because the entire test suite takes .1 seconds and I wasn't worrying about
it?
> > 
> > changed them all to Parallel. Now they take .1 seconds instead of .1
seconds.
> > 
> > \o/
> 
> There's two reasons;
> - With Convey(), you make large test cases.

Oh, that reminds me. I have half of a patch to convey to make them
auto-parallelize that I never committed. I should finish that.

> - You need to set GOMAXPROCS manually in an init() function.

getting fixed in go 1.5!!! woohoo!!

> 
> It's more a statement to make it clear that there is no global context.

yep, very good point. Will add this to my default mental boilerplate.

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:58: /* collections:
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> FTR, I prefer to use // all the time so if I need to comment out a block of
code, it's faster with /* */. Optional nit.

removed in favor of a comment pointing to the README

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:131: num++
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> Remove both num++, insert before line 133:
> num++

done

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:198: defer d.Unlock()
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> Stay symmetrical between the calls.

oops good catch.

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:232: err =
pl.UnmarshalBinary(pdata)
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> merge lines 232 et 233.

done

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:241: d.rwlock.RLock()
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> There will be enough concurrent calls to warrant a RWMutex?

I think there will be. Writes are heavy (esp when they update indexes), and gets
are pretty frequent for most datastore users.

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing
(right):

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.infra_testing:4: 
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> ?

argh

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/memcache.go:29: *wrapper.BrokenFeatures
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> Why a pointer?

gone

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/memcache.go:64: m.data.casID++
On 2015/05/28 at 22:42:38, M-A Ruel wrote:
> Not thread safe.

renamed method, only (supposed) to be called from a locked context. had to add
lock in Set() to make this true.

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/taskqueue.go (right):

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:38: wrapper.DummyTQ(),
On 2015/05/28 at 22:42:39, M-A Ruel wrote:
> you create a TQ everytime?

it returns a constant object (a named struct{}) initialized once in
wrapper/dummy.go. If that's not getting inlined then (╯°□°)╯︵ ┻━┻

https://codereview.chromium.org/1152383003/diff/240001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/taskqueue.go:136: 
On 2015/05/28 at 22:42:39, M-A Ruel wrote:
> Isn't a lock missing here?

indeed

[M-A Ruel, 2015-05-29 02:01:24]

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/README.md:6:
===============================
You should use ----

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/README.md:40: =========================
You should use ----

I also like 2 empty lines before and 1 after.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/README.md:46: bob: 1
If you want it to render inconditionally properly, I think you need +4. Maybe
GoB is happy with +2.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:31: ret := k.knrFunc
if k.knrFunc == nil {
  k.knrFunc = goond.DefaultKindName
}
return k.knrFunc

would be more readable

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:39: func (k *knrKeeper)
SetKindNameResolver(knr goon.KindNameResolver) {
What's the use case?

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:99: err =
numData.UnmarshalBinary(v)
if v := ents.Get(key); v != nil {
  numData := &propertyList{}
  if err := numData.UnmarshalBinary(v); err != nil {
    return 0, err
  }
  return (*numData)[0].Value.(int64), nil
}
return 0, nil

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:110: if v == nil {
if v := ents.Get(key); v != nil {

then switch groups below.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:171: if err != nil {
if key, err = d.putInner(key, plData); err != nil {

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:190: 
I don't think this much spacing is needed here and other similar functions.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:332:
*wrapper.BrokenFeatures
Does it need to be a pointer?

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:60: err :=
(*gkvlite.Collection)(bc).Set(k, v)
combine lines

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:75: err :=
(*gkvlite.Collection)(bc).VisitItemsAscend(target, withValue, visitor)
here too, and 82

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:89: numItems, numBytes,
err := (*gkvlite.Collection)(bc).GetTotals()
This code doesn't do what you think it does.

Looks like a bug.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:145: tok.kind, err = readString(buf)
group lines 145-146

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:149: tok.stringID, err =
readString(buf)
same

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:154: tok.intID, err =
funnybase.ReadUint(buf)
same

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:198: // see if a key is valid?
Can't parse sarcasm. :)

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:202: // since we do "client-side"
validataion of namespaces, check this here.
validation

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:233: testKey := k
Remove

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:235: testKey = newKey(ns, k.Kind(),
"", 1, k.Parent())
k = newKey(ns, k.Kind(), "", 1, k.Parent())

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/key.go:237: return KeyValid(ns, testKey,
opt)
return KeyValid(ns, k, opt)

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/memcache.go:40: lck := sync.Mutex{}
Sometimes you use var err error, sometimes this form. Not a big deal, just
noting the fact you are internally inconsistent on this.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/memcache.go:177: delete(m.data.items, key)
It seems inefficient to look at expiration just to delete a key. So instead do:

if _, ok := m.data.items[key]; ok {


it's silly that memcache.ErrCacheMiss is returned on Delete(), users shouldn't
care. But if the official impl does that, it's fine to be consistent with it.

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://chromiumcodereview.appspot.com/1152383003/diff/260001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memory/plist.go:360: err := v.WriteBinary(buf)
merge lines

[iannucci, 2015-05-29 02:42:29]

ptal again...

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:6:
===============================
On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> You should use ----

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:40: =========================
On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> You should use ----
> 
> I also like 2 empty lines before and 1 after.

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:46: bob: 1
On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> If you want it to render inconditionally properly, I think you need +4. Maybe
GoB is happy with +2.

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:31: ret := k.knrFunc
On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> if k.knrFunc == nil {
>   k.knrFunc = goond.DefaultKindName
> }
> return k.knrFunc
> 
> would be more readable

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:39: func (k *knrKeeper)
SetKindNameResolver(knr goon.KindNameResolver) {
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> What's the use case?

It's the function that can take an object-to-be-serialized and returns the Kind
for it. By default it uses reflection to find the name, but sometimes you want
to override it. It's documented here: http://godoc.org/github.com/mjibson/goon

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:99: err =
numData.UnmarshalBinary(v)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> if v := ents.Get(key); v != nil {
>   numData := &propertyList{}
>   if err := numData.UnmarshalBinary(v); err != nil {
>     return 0, err
>   }
>   return (*numData)[0].Value.(int64), nil
> }
> return 0, nil

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:110: if v == nil {
On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> if v := ents.Get(key); v != nil {
> 
> then switch groups below.

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:171: if err != nil {
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> if key, err = d.putInner(key, plData); err != nil {

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:190: 
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> I don't think this much spacing is needed here and other similar functions.

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:332:
*wrapper.BrokenFeatures
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> Does it need to be a pointer?

yes

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:60: err :=
(*gkvlite.Collection)(bc).Set(k, v)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> combine lines

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:75: err :=
(*gkvlite.Collection)(bc).VisitItemsAscend(target, withValue, visitor)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> here too, and 82

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:89: numItems, numBytes,
err := (*gkvlite.Collection)(bc).GetTotals()
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> This code doesn't do what you think it does.
> 

What do you think I think it does?

> Looks like a bug.

The tests show it working fine...

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/key.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:145: tok.kind, err = readString(buf)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> group lines 145-146

done.

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:149: tok.stringID, err =
readString(buf)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> same

done..

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:154: tok.intID, err =
funnybase.ReadUint(buf)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> same

done...

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:198: // see if a key is valid?
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> Can't parse sarcasm. :)

add /s :)

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:202: // since we do "client-side"
validataion of namespaces, check this here.
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> validation

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/key.go:237: return KeyValid(ns, testKey,
opt)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> return KeyValid(ns, k, opt)

done

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/memcache.go:40: lck := sync.Mutex{}
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> Sometimes you use var err error, sometimes this form. Not a big deal, just
noting the fact you are internally inconsistent on this.

.... I always prefer `foo := something{}`, but you had me change the other
locations.

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/memcache.go:177: delete(m.data.items, key)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> It seems inefficient to look at expiration just to delete a key. So instead
do:
> 
> if _, ok := m.data.items[key]; ok {
> 
> 
> it's silly that memcache.ErrCacheMiss is returned on Delete(), users shouldn't
care. But if the official impl does that, it's fine to be consistent with it.

It does seem silly, but that's actually how memcache expiration works.

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/plist.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/plist.go:360: err := v.WriteBinary(buf)
On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> merge lines

done.....

[iannucci, 2015-05-29 02:44:01]

On 2015/05/29 at 02:42:29, iannucci wrote:
> ptal again...
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/README.md (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/README.md:6:
===============================
> On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> > You should use ----
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/README.md:40: =========================
> On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> > You should use ----
> > 
> > I also like 2 empty lines before and 1 after.
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/README.md:46: bob: 1
> On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> > If you want it to render inconditionally properly, I think you need +4.
Maybe GoB is happy with +2.
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:31: ret := k.knrFunc
> On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> > if k.knrFunc == nil {
> >   k.knrFunc = goond.DefaultKindName
> > }
> > return k.knrFunc
> > 
> > would be more readable
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:39: func (k *knrKeeper)
SetKindNameResolver(knr goon.KindNameResolver) {
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > What's the use case?
> 
> It's the function that can take an object-to-be-serialized and returns the
Kind for it. By default it uses reflection to find the name, but sometimes you
want to override it. It's documented here:
http://godoc.org/github.com/mjibson/goon
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:99: err =
numData.UnmarshalBinary(v)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > if v := ents.Get(key); v != nil {
> >   numData := &propertyList{}
> >   if err := numData.UnmarshalBinary(v); err != nil {
> >     return 0, err
> >   }
> >   return (*numData)[0].Value.(int64), nil
> > }
> > return 0, nil
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:110: if v == nil {
> On 2015/05/29 at 02:01:22, M-A Ruel wrote:
> > if v := ents.Get(key); v != nil {
> > 
> > then switch groups below.
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:171: if err != nil {
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > if key, err = d.putInner(key, plData); err != nil {
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:190: 
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > I don't think this much spacing is needed here and other similar functions.
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:332:
*wrapper.BrokenFeatures
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > Does it need to be a pointer?
> 
> yes
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:60: err :=
(*gkvlite.Collection)(bc).Set(k, v)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > combine lines
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:75: err :=
(*gkvlite.Collection)(bc).VisitItemsAscend(target, withValue, visitor)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > here too, and 82
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:89: numItems, numBytes,
err := (*gkvlite.Collection)(bc).GetTotals()
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > This code doesn't do what you think it does.
> > 
> 
> What do you think I think it does?
> 
> > Looks like a bug.
> 
> The tests show it working fine...
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/key.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:145: tok.kind, err =
readString(buf)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > group lines 145-146
> 
> done.
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:149: tok.stringID, err =
readString(buf)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > same
> 
> done..
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:154: tok.intID, err =
funnybase.ReadUint(buf)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > same
> 
> done...
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:198: // see if a key is valid?
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > Can't parse sarcasm. :)
> 
> add /s :)
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:202: // since we do "client-side"
validataion of namespaces, check this here.
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > validation
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/key.go:237: return KeyValid(ns, testKey,
opt)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > return KeyValid(ns, k, opt)
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/memcache.go:40: lck := sync.Mutex{}
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > Sometimes you use var err error, sometimes this form. Not a big deal, just
noting the fact you are internally inconsistent on this.
> 
> .... I always prefer `foo := something{}`, but you had me change the other
locations.

(for non-integral types)

> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/memcache.go:177: delete(m.data.items,
key)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > It seems inefficient to look at expiration just to delete a key. So instead
do:
> > 
> > if _, ok := m.data.items[key]; ok {
> > 
> > 
> > it's silly that memcache.ErrCacheMiss is returned on Delete(), users
shouldn't care. But if the official impl does that, it's fine to be consistent
with it.
> 
> It does seem silly, but that's actually how memcache expiration works.
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/plist.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/plist.go:360: err := v.WriteBinary(buf)
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > merge lines
> 
> done.....

[M-A Ruel, 2015-05-29 16:16:19]

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:27: func (bs *memStore)
Snapshot() *memStore {
From what I understand, these functions are exported to match the gkvlite.Store
but are not meant for external use, right?

https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:89: numItems, numBytes,
err := (*gkvlite.Collection)(bc).GetTotals()
On 2015/05/29 02:42:28, iannucci wrote:
> On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > This code doesn't do what you think it does.
> > 
> 
> What do you think I think it does?
> 
> > Looks like a bug.
> 
> The tests show it working fine...

I thought it aliased the variables but it looks like I was wrong. The test was
also hard to find.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:2: ---------------------------
This should still be H1 so ====

http://daringfireball.net/projects/markdown/syntax#header

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/binutils.go:70: func btoi(b bool) byte {
This function is not used anywhere, remove.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:37: "TQ": 0,
Did you plan to have this list be dynamic? Otherwise you could use a type with
const, e.g.

type ContextType string
const TQ ContextType = "TQ"

but then you don't even need strings anymore and would just use indexes, so no
need for memContextIndices. I'd prefer that.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:48: type dataStoreData
struct {
Could you describe why dataStoreData exposes sync.Locker?

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:233: d.Lock()
I'd prefer to always use d.rwlock.Lock() for consistency.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_test.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_test.go:226: num, _ :=
ents.GetTotals()
Why don't you check numBytes too? At least assert a range if non deterministic?

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go:31: func
SetStructKey(src interface{}, key *datastore.Key, knr goon.KindNameResolver)
error {
does it need to be exported?

[iannucci, 2015-05-29 16:33:11]

iannucci@chromium.org changed reviewers:
+ vadimsh@chromium.orgp
- vadimsh@chromium.org

[iannucci, 2015-05-29 16:33:14]

ptal again

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/README.md (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/README.md:2: ---------------------------
On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> This should still be H1 so ====
> 
> http://daringfireball.net/projects/markdown/syntax#header

done

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/binutils.go:70: func btoi(b bool) byte {
On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> This function is not used anywhere, remove.

it's used in one of the next CLs (one of the 6 CLs based on this one that I need
to rebase after every iteration on this review). removed from this one.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/context.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/context.go:37: "TQ": 0,
On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> Did you plan to have this list be dynamic? Otherwise you could use a type with
const, e.g.
> 
> type ContextType string
> const TQ ContextType = "TQ"
> 
> but then you don't even need strings anymore and would just use indexes, so no
need for memContextIndices. I'd prefer that.

sgtm

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:48: type dataStoreData
struct {
On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> Could you describe why dataStoreData exposes sync.Locker?

added a note that this implements memContextObj.

It's needed so that we can lock taskqueue + datastore at the same time in
RunInTransaction.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:233: d.Lock()
On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> I'd prefer to always use d.rwlock.Lock() for consistency.

done

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_test.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_test.go:226: num, _ :=
ents.GetTotals()
On 2015/05/29 at 16:16:19, M-A Ruel wrote:
> Why don't you check numBytes too? At least assert a range if non
deterministic?

because I don't care about the number of bytes, I care about the number of
entities. See the TODO which is 2 lines up.

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go (right):

https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go:31: func
SetStructKey(src interface{}, key *datastore.Key, knr goon.KindNameResolver)
error {
On 2015/05/29 at 16:16:19, M-A Ruel wrote:
> does it need to be exported?

yes because otherwise it would be useless. you had me move this into an internal
package, remember?

[iannucci, 2015-05-29 16:35:01]

On 2015/05/29 at 16:16:19, maruel wrote:
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:27: func (bs *memStore)
Snapshot() *memStore {
> From what I understand, these functions are exported to match the
gkvlite.Store but are not meant for external use, right?

yes, there's no way to get the mem{Store,Collection} objects, and I would like
to keep the method names identical, even though their signatures change.

un-capping them would also lead to weird methods like `delete`, which is a
builtin.

> 
>
https://codereview.chromium.org/1152383003/diff/260001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/gkvlite_utils.go:89: numItems, numBytes,
err := (*gkvlite.Collection)(bc).GetTotals()
> On 2015/05/29 02:42:28, iannucci wrote:
> > On 2015/05/29 at 02:01:23, M-A Ruel wrote:
> > > This code doesn't do what you think it does.
> > > 
> > 
> > What do you think I think it does?
> > 
> > > Looks like a bug.
> > 
> > The tests show it working fine...
> 
> I thought it aliased the variables but it looks like I was wrong. The test was
also hard to find.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/README.md (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/README.md:2: ---------------------------
> This should still be H1 so ====
> 
> http://daringfireball.net/projects/markdown/syntax#header
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/binutils.go:70: func btoi(b bool) byte {
> This function is not used anywhere, remove.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/context.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/context.go:37: "TQ": 0,
> Did you plan to have this list be dynamic? Otherwise you could use a type with
const, e.g.
> 
> type ContextType string
> const TQ ContextType = "TQ"
> 
> but then you don't even need strings anymore and would just use indexes, so no
need for memContextIndices. I'd prefer that.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:48: type dataStoreData
struct {
> Could you describe why dataStoreData exposes sync.Locker?
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:233: d.Lock()
> I'd prefer to always use d.rwlock.Lock() for consistency.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_test.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_test.go:226: num, _ :=
ents.GetTotals()
> Why don't you check numBytes too? At least assert a range if non
deterministic?
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go:31: func
SetStructKey(src interface{}, key *datastore.Key, knr goon.KindNameResolver)
error {
> does it need to be exported?

[iannucci, 2015-05-29 16:37:11]

On 2015/05/29 at 16:33:14, iannucci wrote:
> ptal again
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/README.md (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/README.md:2: ---------------------------
> On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> > This should still be H1 so ====
> > 
> > http://daringfireball.net/projects/markdown/syntax#header
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/binutils.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/binutils.go:70: func btoi(b bool) byte {
> On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> > This function is not used anywhere, remove.
> 
> it's used in one of the next CLs (one of the 6 CLs based on this one that I
need to rebase after every iteration on this review). removed from this one.

I got rid of this function in the downstream CLs too. turns out it wasn't really
helpful for readability anyway.

> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/context.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/context.go:37: "TQ": 0,
> On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> > Did you plan to have this list be dynamic? Otherwise you could use a type
with const, e.g.
> > 
> > type ContextType string
> > const TQ ContextType = "TQ"
> > 
> > but then you don't even need strings anymore and would just use indexes, so
no need for memContextIndices. I'd prefer that.
> 
> sgtm
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:48: type dataStoreData
struct {
> On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> > Could you describe why dataStoreData exposes sync.Locker?
> 
> added a note that this implements memContextObj.
> 
> It's needed so that we can lock taskqueue + datastore at the same time in
RunInTransaction.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:233: d.Lock()
> On 2015/05/29 at 16:16:18, M-A Ruel wrote:
> > I'd prefer to always use d.rwlock.Lock() for consistency.
> 
> done
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_test.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_test.go:226: num, _ :=
ents.GetTotals()
> On 2015/05/29 at 16:16:19, M-A Ruel wrote:
> > Why don't you check numBytes too? At least assert a range if non
deterministic?
> 
> because I don't care about the number of bytes, I care about the number of
entities. See the TODO which is 2 lines up.
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/300001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/internal/goon/goon.go:31: func
SetStructKey(src interface{}, key *datastore.Key, knr goon.KindNameResolver)
error {
> On 2015/05/29 at 16:16:19, M-A Ruel wrote:
> > does it need to be exported?
> 
> yes because otherwise it would be useless. you had me move this into an
internal package, remember?

[M-A Ruel, 2015-05-29 18:16:53]

Without any further suspense ... lgtm!

BTW, you can click "Done" instead of typing "done" every time, this looks
tedious. Especially the "done.." which looked very annoyed. Don't worry, I've
seen worse CLs.

FTR, I'm intentionally anal on larger CLs, especially in this case so we are
clear on the coding style. I think 2-3 liners comment on the structs would go a
long way to clarify the code. Not that I'm either super careful in commenting
either, feel free to raise the issue when reviewing, I tend to forget.

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:60:
////////////////////////////// New(dataStoreData) //////////////////////////////
FTR, I don't see value in these kinds of separators, especially when there's one
ever 15 lines or so.

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:62: func
newDataStoreData() *dataStoreData {
The thing I'd really like for each object is for you to describe which
interfaces they describe. Then reading would be much easier. Right now all your
new*() functions return the underlying type and not an interface. It's fine but
it's harder to figure out the overall context when we don't know what interfaces
it implements. You do list it occasionally, for example at line 71, but that's
not the right place, I'd want to see it at line 48.

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:318:
/////////////////////////////// txnDataStoreData ///////////////////////////////
I don't know if you intended to insert a line in-between.

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/datastore_data.go:345:
/////////////////////// memContextObj(txnDataStoreData) ////////////////////////
I assume you mean that txnDataStoreData implements the interface memContextObj.
This could be a comment at line 323 explaining it implements memContextObj and
wrapper.BrokenFeatures and it'd be clearer.

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):

https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memory/memcache.go:26: // implementation of
{wrapper.Memcache, wrapper.Testable}.
That's a great example that helps understanding the use case of the struct. I
prefer much this compared to the previous files.

[iannucci, 2015-05-29 20:13:36]

On 2015/05/29 at 18:16:53, maruel wrote:
> Without any further suspense ... lgtm!
> 
> BTW, you can click "Done" instead of typing "done" every time, this looks
tedious. Especially the "done.." which looked very annoyed. Don't worry, I've
seen worse CLs.
> 

Ahhh.... I need to file a bug about this for the new polymer UI (I definitely
love the Done button in the old UI)

> FTR, I'm intentionally anal on larger CLs, especially in this case so we are
clear on the coding style. I think 2-3 liners comment on the structs would go a
long way to clarify the code. Not that I'm either super careful in commenting
either, feel free to raise the issue when reviewing, I tend to forget.
> 

Ya, it's fine... it was a big CL :(

>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/datastore_data.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:60:
////////////////////////////// New(dataStoreData) //////////////////////////////
> FTR, I don't see value in these kinds of separators, especially when there's
one ever 15 lines or so.
> 

I was trying to group the implementation of interfaces for a type... though New
is not really an interface.

>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:62: func
newDataStoreData() *dataStoreData {
> The thing I'd really like for each object is for you to describe which
interfaces they describe. Then reading would be much easier. Right now all your
new*() functions return the underlying type and not an interface. It's fine but
it's harder to figure out the overall context when we don't know what interfaces
it implements. You do list it occasionally, for example at line 71, but that's
not the right place, I'd want to see it at line 48.
> 

DOCUMENTED WITH CODE! Compiler verifies comment! :D

>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:318:
/////////////////////////////// txnDataStoreData ///////////////////////////////
> I don't know if you intended to insert a line in-between.
> 
>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/datastore_data.go:345:
/////////////////////// memContextObj(txnDataStoreData) ////////////////////////
> I assume you mean that txnDataStoreData implements the interface
memContextObj. This could be a comment at line 323 explaining it implements
memContextObj and wrapper.BrokenFeatures and it'd be clearer.
> 
>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/wrapper/memory/memcache.go (right):
> 
>
https://codereview.chromium.org/1152383003/diff/320001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/wrapper/memory/memcache.go:26: // implementation of
{wrapper.Memcache, wrapper.Testable}.
> That's a great example that helps understanding the use case of the struct. I
prefer much this compared to the previous files.

[M-A Ruel, 2015-05-29 20:37:37]

lgtm

[iannucci, 2015-05-29 20:41:16]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-05-29 20:41:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152383003/410001

[commit-bot: I haz the power, 2015-05-29 20:46:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-29 20:46:02]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[iannucci, 2015-05-29 20:57:41]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-05-29 20:57:42]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1152383003/#ps430001
(title: "fix goop")

[commit-bot: I haz the power, 2015-05-29 20:57:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152383003/430001

[commit-bot: I haz the power, 2015-05-29 21:04:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-29 21:04:10]

Try jobs failed on following builders:
  infra_tester on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/infra_tester/bu...)

[iannucci, 2015-05-29 21:12:39]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-05-29 21:12:40]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org
Link to the patchset: https://codereview.chromium.org/1152383003/#ps450001
(title: "add go-slab dependency")

[commit-bot: I haz the power, 2015-05-29 21:12:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1152383003/450001

[commit-bot: I haz the power, 2015-05-29 21:16:42]

Committed patchset #24 (id:450001) as
https://chromium.googlesource.com/infra/infra/+/f4e4857179f3e4f94f224cde281ae...