Refactor current GAE abstraction library to be free of the SDK*

Refactor current GAE abstraction library to be free of the SDK*

Also improves the struct serialization implementation over the SDK's:
  * 'non-struct' representation is now map[string][]values, instead of
    []PropertyList. This makes working with PropertyLoadSaver much easier.
  * Fields can implement a DSPropertyConverter interface to transparently be
    serialized to/from some other datastore-compatible type. This makes many
    uses of PropertyLoadSaver unnecessary.
  * Lots of reflection code moved out of structPLS.{Load,Save} into the creation
    of the struct codec (e.g. it happens once instead of on every load/save now).
  * Struct codecs are directly accessible by users, allowing easier
    implementation of PropertyLoadSaver interface.
  * Struct codecs support 'special' tagged fields. This will be used for
    implementing an embedded-key Datastore interface without adding any extra
    reflection code.
  * DSProperty's now have strictly private fields. This ensures that every
    DSProperty instance contains only valid types for Value (since it's not
    possible to construct one with an incompatible Value now), which simplifies
    error checking code.
  * DSProperty does automatic upconversion of close-but-not-exact types (like
    MyString -> string, and int -> int64). This upconversion was previously only
    available for struct serialization.
  * DSProperty's now retain a sortable 'Type' field to avoid unnecessary
    additional reflection when serializing them (such as to memcache).
  * There is now a consistent, sortable binary serialization format for
    DSPropertyMap's, which will be used for the follow-on transparent memcache
    implementation. It's also used internally by the in-memory testing
    implementation of the datastore.

*I alias some error types from the new-SDK, but users of the abstraction no
longer need to import the SDK at all.

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

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

[iannucci, 2015-07-07 05:00:32]

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

[iannucci, 2015-07-07 05:00:45]

On 2015/07/07 04:59:51, iannucci wrote:

Note that this depends on https://chromiumcodereview.appspot.com/1219323002/

[Vadim Sh., 2015-07-13 21:17:12]

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/context.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/context.go:12: rawDatastoreKey key = 1
nit: 

const (
  rawDatastoreKey key = iota
  memcacheKey key = iota
  ...
)

(iota exists for some reason... might as well use it)

?

I don't insist on this change.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore.go:11: "infra/gae/libs/gae"
nit: group non-stdlib imports separately

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:21: const
maxIndexedProperties = 20000
O_O 
is it a real number? Does DS really support 20000 properties?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:62: entityType = "int"
can this be extracted using reflect? e.g. reflect.TypeOf(val).Type() (or
whatever, I'm not familiar with reflect).

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:96: "cannot load field %q
into a %q: %s", name, t, reason))
is it ok that fatal is not set here?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:103: func loadInner(codec
*structCodec, structValue reflect.Value, index int, name string, p
gae.DSProperty, requireSlice bool) string {
why not return "error" type? (and convert convFailures to []error)

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:120: v.Set(reflect.Append(v,
reflect.New(v.Type().Elem()).Elem()))
ugh...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:358: for _, s := range
strings.Split(name, ".") {
use regexp?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:396: problem:  
errRecursiveStruct, // we'll clear this later if it's not
if its' not what?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:399: // If the codec has a
problem, free up the indexes
is it micro memory optimization?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_key.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:5: // adapted from
github.com/golang/appengine/datastore
not "HEAVILY" this time? :)

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:13:
"github.com/golang/protobuf/proto"
nit: move github import to a separate group

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:131: if k == nil {
you know that interface is nil iff both value part and type part are nil? e.g.

var impl *dsKeyImpl = nil
(gae.DSKey)(impl) != nil // true

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:225: inner(k.Parent())
why not loop like other functions in this module?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File
go/src/infra/gae/libs/gae/helper/internal/protos/datastore/datastore_v3.proto
(right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/internal/protos/datastore/datastore_v3.proto:1:
syntax = "proto2";
provide instructions for how to update this file.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/serialize.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:27: var
ReadDSPropertyMapReasonableLimit uint64 = 30000
why not const? Same below.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:36: // no reason to redundantly
encode them.
IMHO, there's no reason to complicate API because of this. What not always
encode them? IIUC, python impl always encode them.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:164: // This method truncates the
time to microseconds and drops the timezone,
maybe panic if timezone is not UTC\default?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/shell_key.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/shell_key.go:11: type shellDSKey struct {
what does "shell" stand for?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/shell_key.go:25: copy(newToks, toks)
nit: newToks := append(nil, toks...)? :) It's probably smart enough to
preallocate the array.

Also, why copy? DKKeyTok is supposed to be immutable. Avoiding copy will make
.Parent() a bit faster (and .Parent() is used very often).

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memcache.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:15: // implementation constraints. They
return the oririnal item to allow easy
typo: oririnal

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:25: SetKey(string) MCItem
No errors?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:43: type MCCodec interface {
MCCodecDef vs MCCodec is odd... 

Does MCCoded have to be a public interface? 

How about:

type MemcacheEssentials {
  Add(item MCItem) error
  ...
}

type Memcache interface {
  MemcacheEssentials
  
  NewItem(...)
  Delete() // :( 
}

Or maybe merge InflateCodec into MCFactory somehow and get rid of MCCoded
interface (in favor of Memcache interface).

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:55: // Create a new, empty, MCItem
nit: NewItem creates a new ...

Or just remove the comment.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memory/README.md (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/README.md:1: In-memory appengine wrapper
I gave up reviewing at this point... Will continue later.

[iannucci, 2015-07-13 22:39:53]

Thanks for the review! Should I remove the MCCodec crap entirely?

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/context.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/context.go:12: rawDatastoreKey key = 1
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> nit: 
> 
> const (
>   rawDatastoreKey key = iota
>   memcacheKey key = iota
>   ...
> )
> 
> (iota exists for some reason... might as well use it)
> 
> ?
> 
> I don't insist on this change.

Unfortunately they need to be var's so that they have a real type... otherwise
they transparently turn into 'int' and then when they're used in the context as
keys they're not unique any more :(

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore.go:11: "infra/gae/libs/gae"
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> nit: group non-stdlib imports separately

blargh I thought I got all these

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_impl.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:21: const
maxIndexedProperties = 20000
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> O_O 
> is it a real number? Does DS really support 20000 properties?

Yep...
https://github.com/golang/appengine/blob/b667a5000b082e49c6c6d16867d376a12e94...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:62: entityType = "int"
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> can this be extracted using reflect? e.g. reflect.TypeOf(val).Type() (or
> whatever, I'm not familiar with reflect).

Yeah, now that I look at this... I'm not sure what they were thinking:
https://github.com/golang/appengine/blob/b667a5000b082e49c6c6d16867d376a12e94...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:96: "cannot load field %q
into a %q: %s", name, t, reason))
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> is it ok that fatal is not set here?

Yeah, these are just conversion errors. In the SDK, there's only one error, and
it could be fatal OR a conversion error (and the SDK only preserves the last
conversion error seen).

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:103: func loadInner(codec
*structCodec, structValue reflect.Value, index int, name string, p
gae.DSProperty, requireSlice bool) string {
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> why not return "error" type? (and convert convFailures to []error)

I do this refactor in a later CL actually (though not quite at this location). I
change the signature of Load to only return a single error, which may either be
a MultiError of conversions failures, or a single fatal error.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:120: v.Set(reflect.Append(v,
reflect.New(v.Type().Elem()).Elem()))
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> ugh...

yep
https://github.com/golang/appengine/blob/b667a5000b082e49c6c6d16867d376a12e94...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:358: for _, s := range
strings.Split(name, ".") {
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> use regexp?

Overkill? Also this is verbatim:
https://github.com/golang/appengine/blob/b667a5000b082e49c6c6d16867d376a12e94...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:396: problem:  
errRecursiveStruct, // we'll clear this later if it's not
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> if its' not what?

if it's not actually recursive. Fixed comment.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_impl.go:399: // If the codec has a
problem, free up the indexes
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> is it micro memory optimization?

yes, but I also don't want a codec's inner fields to be used accidentally if the
codec is only half-way initialized. This way we'll get nil-panics if something
(somehow) uses a partially initialized codec.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/datastore_key.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:5: // adapted from
github.com/golang/appengine/datastore
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> not "HEAVILY" this time? :)

Yeah most of this is copy-paste from here:
https://github.com/golang/appengine/blob/b667a5000b082e49c6c6d16867d376a12e94...

(though some of the functions like the DSKeySplit are written by me)

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:13:
"github.com/golang/protobuf/proto"
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> nit: move github import to a separate group

done

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:131: if k == nil {
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> you know that interface is nil iff both value part and type part are nil? e.g.
> 
> var impl *dsKeyImpl = nil
> (gae.DSKey)(impl) != nil // true

yes I know. It's technically possible for (*dsKeyImpl)(nil) to implement
gae.DSKey too :P

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/datastore_key.go:225: inner(k.Parent())
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> why not loop like other functions in this module?

I could... yeah fine I'll do that. Didn't want to have to deal with the index
reversal, but you're right, it's more consistent.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File
go/src/infra/gae/libs/gae/helper/internal/protos/datastore/datastore_v3.proto
(right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/internal/protos/datastore/datastore_v3.proto:1:
syntax = "proto2";
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> provide instructions for how to update this file.

Added README.md in this folder

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/serialize.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:27: var
ReadDSPropertyMapReasonableLimit uint64 = 30000
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> why not const? Same below.

Done.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:31: var
ReadKeyNumToksReasonableLimit uint64 = 50
Done.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:36: // no reason to redundantly
encode them.
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> IMHO, there's no reason to complicate API because of this. What not always
> encode them? IIUC, python impl always encode them.

Bleh... I hate redundant information... This could easily double or triple the
size of a single simple index :/ Do I have to? I already burned a byte to make
this safe to mix and match with/without context encode/decode functions....

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:164: // This method truncates the
time to microseconds and drops the timezone,
On 2015/07/13 21:17:11, Vadim Sh. wrote:
> maybe panic if timezone is not UTC\default?

sgtm, done.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/shell_key.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/shell_key.go:11: type shellDSKey struct {
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> what does "shell" stand for?

it's.. not a very good name. I'll change it. Made it public as well, since it's
useful to have a concrete type.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/shell_key.go:25: copy(newToks, toks)
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> nit: newToks := append(nil, toks...)? :) It's probably smart enough to
> preallocate the array.
> 
> Also, why copy? DKKeyTok is supposed to be immutable. Avoiding copy will make
> .Parent() a bit faster (and .Parent() is used very often).

I guess I'm just paranoid of re-using the slice because the caller technically
has pointers to all the stuff inside of it (and who knows where the caller got
the slice from in the first place). The SDK implementation uses a
pseudo-linked-list for this (e.g. every key is a token with a copy of
appid+namespace and a pointer to the parent).

However, since I copy it here, I don't need to copy it in Parent.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memcache.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:15: // implementation constraints. They
return the oririnal item to allow easy
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> typo: oririnal

Done.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:25: SetKey(string) MCItem
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> No errors?

Correct, this is replacing a dumb struct (with a single f'ing hidden field which
makes it useless), so these Set* should never fail.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:43: type MCCodec interface {
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> MCCodecDef vs MCCodec is odd... 
> 
> Does MCCoded have to be a public interface? 
> 
> How about:
> 
> type MemcacheEssentials {
>   Add(item MCItem) error
>   ...
> }
> 
> type Memcache interface {
>   MemcacheEssentials
>   
>   NewItem(...)
>   Delete() // :( 
> }
> 
> Or maybe merge InflateCodec into MCFactory somehow and get rid of MCCoded
> interface (in favor of Memcache interface).

With your permission, I'd like to drop the codec thing entirely... We can add a
sane alternative later if we really need it? I was just doing it for
completeness.

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:55: // Create a new, empty, MCItem
On 2015/07/13 21:17:12, Vadim Sh. wrote:
> nit: NewItem creates a new ...
> 
> Or just remove the comment.

removed, it's pretty obvious what this does.

[Vadim Sh., 2015-07-14 00:12:37]

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/serialize.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:36: // no reason to redundantly
encode them.
On 2015/07/13 22:39:52, iannucci wrote:
> On 2015/07/13 21:17:11, Vadim Sh. wrote:
> > IMHO, there's no reason to complicate API because of this. What not always
> > encode them? IIUC, python impl always encode them.
> 
> Bleh... I hate redundant information... This could easily double or triple the
> size of a single simple index :/ Do I have to? I already burned a byte to make
> this safe to mix and match with/without context encode/decode functions....

okay...

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memcache.go (right):

https://codereview.chromium.org/1222903002/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memcache.go:43: type MCCodec interface {
On 2015/07/13 22:39:53, iannucci wrote:
> On 2015/07/13 21:17:12, Vadim Sh. wrote:
> > MCCodecDef vs MCCodec is odd... 
> > 
> > Does MCCoded have to be a public interface? 
> > 
> > How about:
> > 
> > type MemcacheEssentials {
> >   Add(item MCItem) error
> >   ...
> > }
> > 
> > type Memcache interface {
> >   MemcacheEssentials
> >   
> >   NewItem(...)
> >   Delete() // :( 
> > }
> > 
> > Or maybe merge InflateCodec into MCFactory somehow and get rid of MCCoded
> > interface (in favor of Memcache interface).
> 
> With your permission, I'd like to drop the codec thing entirely... We can add
a
> sane alternative later if we really need it? I was just doing it for
> completeness. 

sgtm

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/helper/serialize.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/helper/serialize.go:169: panic(fmt.Errorf("helper: UTC
OR DEATH: %s", t))
:)

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memory/datastore_query.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/datastore_query.go:345: "on the current
namespace")
MADE UP ERROR?..

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/datastore_query.go:361: "Only ancestor queries
are allowed inside transactions")
nit: errors must be lowercase apparently

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

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/memcache.go:220: func (m *memcacheImpl)
retrieve(key string) (*mcItem, bool) {
nit: retrieveLocked

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memory/raw_datstore.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/raw_datstore.go:10: "infra/gae/libs/gae"
nit: move below

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/raw_datstore.go:60: return
helper.NewDSKey("dev~app", d.ns, kind, stringID, intID, parent)
do you have memory mock for GlobalInfo? Use app id stored there.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:14: type dsKeyImpl struct {
any reason it can't be

type dsKeyImpl *datastore.Key?

Or 

type dsKeyImpl struct {
  datastore.Key
}

Point to pointer is sort of bad.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:21: func (k dsKeyImpl) Parent()
gae.DSKey          { return &dsKeyImpl{k.Key.Parent()} }
f2r(k.Key.Parent())?

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:32: if err != nil {
huh?

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:42: func mr2f(ks
[]*datastore.Key) []gae.DSKey {
these are very cryptic names :)

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/memcache.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/memcache.go:8: "golang.org/x/net/context"
move below

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/raw_datastore_filter.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/raw_datastore_filter.go:9: "infra/gae/libs/gae"
nit: new line before it

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/taskqueue.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/taskqueue.go:9: "golang.org/x/net/context"
nit: move below

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/taskqueue.go:77: func mo2ne(os []*taskqueue.Task,
err error) ([]*gae.TQTask, error) {
cryptic names again.. :(

Write at least one comment explaining what various letters stand for.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/m...
File go/src/infra/gae/libs/memlock/memlock.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/m...
go/src/infra/gae/libs/memlock/memlock.go:14: "golang.org/x/net/context"
move it below

[iannucci, 2015-07-14 01:07:46]

more fixes

I thought that golang.org/x/* belonged in the first group?

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memory/datastore_query.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/datastore_query.go:345: "on the current
namespace")
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> MADE UP ERROR?..

I don't know what the real error text is in this instance, but I know it's an
error. Just wanted to make sure
no one tried to match on this error text or something.

That said, I gave up on the whole BAD_REQUEST shit, so I'll fix it :)

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/datastore_query.go:361: "Only ancestor queries
are allowed inside transactions")
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> nit: errors must be lowercase apparently

I think this one is an actual error message tho :p. Done.

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

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/memcache.go:220: func (m *memcacheImpl)
retrieve(key string) (*mcItem, bool) {
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> nit: retrieveLocked

good catch. Done.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/memory/raw_datstore.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/raw_datstore.go:10: "infra/gae/libs/gae"
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> nit: move below

Done.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/memory/raw_datstore.go:60: return
helper.NewDSKey("dev~app", d.ns, kind, stringID, intID, parent)
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> do you have memory mock for GlobalInfo? Use app id stored there.

yeah... turns out that this is hard (and AppID is not modifiable through the API
anyway). I could do it but it makes a lot of the internal apis more complex. I
made it a constant though (which is now consistent with GlobalInfo.AppID()'s
return value).

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:14: type dsKeyImpl struct {
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> any reason it can't be
> 
> type dsKeyImpl *datastore.Key?
> 
> Or 
> 
> type dsKeyImpl struct {
>   datastore.Key
> }
> 
> Point to pointer is sort of bad.

Note that it's all bare-struct receivers below, so it shouldn't ever be a
pointer-to-a-pointer. I also felt uncomfortable about doing *k where k is a
*datastore.Key (since the SDK never handles a dereferenced Key object). I did
have some superfluous &'s below though, which I've removed.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:21: func (k dsKeyImpl) Parent()
gae.DSKey          { return &dsKeyImpl{k.Key.Parent()} }
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> f2r(k.Key.Parent())?

r2f, but yes

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:32: if err != nil {
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> huh?

it's for wrapping methods which return (*datastore.Key, error). added comment.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/datastore_key.go:42: func mr2f(ks
[]*datastore.Key) []gae.DSKey {
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> these are very cryptic names :)

yeah, added comments

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/memcache.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/memcache.go:8: "golang.org/x/net/context"
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> move below

oh really? I've been including this one as a 'standard' import. I think that's
goimports' behavior too?

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/raw_datastore_filter.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/raw_datastore_filter.go:9: "infra/gae/libs/gae"
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> nit: new line before it

Done.

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/prod/taskqueue.go (right):

https://codereview.chromium.org/1222903002/diff/80001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/prod/taskqueue.go:77: func mo2ne(os []*taskqueue.Task,
err error) ([]*gae.TQTask, error) {
On 2015/07/14 00:12:37, Vadim Sh. wrote:
> cryptic names again.. :(
> 
> Write at least one comment explaining what various letters stand for.

Done.

[Vadim Sh., 2015-07-14 01:16:28]

lgtm

but I only glanced over hard parts... so if there are bugs they are on your
conscience (for sending giant CLs with adapted code without clearly marking
what's adapted, what's not...).

https://codereview.chromium.org/1222903002/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):

https://codereview.chromium.org/1222903002/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/gae/prod/datastore_key.go:35: //   return
dsR2FErr(datastore.Put(...))
hm, I didn't know golang supports this syntax

[iannucci, 2015-07-14 01:26:41]

On 2015/07/14 01:16:28, Vadim Sh. wrote:
> lgtm
> 
> but I only glanced over hard parts... so if there are bugs they are on your
> conscience (for sending giant CLs with adapted code without clearly marking
> what's adapted, what's not...).

duly noted, thank you for the review despite my dumbness :) The rest of the
reviews are much smaller.

> 
>
https://codereview.chromium.org/1222903002/diff/100001/go/src/infra/gae/libs/...
> File go/src/infra/gae/libs/gae/prod/datastore_key.go (right):
> 
>
https://codereview.chromium.org/1222903002/diff/100001/go/src/infra/gae/libs/...
> go/src/infra/gae/libs/gae/prod/datastore_key.go:35: //   return
> dsR2FErr(datastore.Put(...))
> hm, I didn't know golang supports this syntax

scary isn't it?

[iannucci, 2015-07-14 01:26:49]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-14 01:27:05]

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

[commit-bot: I haz the power, 2015-07-14 01:30:45]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/infra/infra/+/a91c4ac2149f8bbe3711c4c662b6d...