Better attempt at an appengine wrapper.

Better attempt at an appengine wrapper.

Includes implementation using old-style appengine, and will be able to be
expanded to include support for new-style appengine SDK. I also have a partial
in-memory testing implementation in the wings which will be a followup CL.

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

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

[iannucci, 2015-05-23 22:01:22]

PTAL.

Unfortunately we're tied to the old 'appengine' types right now. AFAIK, there's
no way to convert between *"appengine/datastore".Key and
*"google.golang.org/appengine/datastore".Key (without unsafe), because the
latter require an opaque Context. At least with the old-style types I can fake
appengine.Context to fill in e.g. Namespace and AppID fields.

So, using the old types will allow us to make an implementation using the new
SDK, but using the new types would prevent us from making an implementation
using the new SDK.

[iannucci, 2015-05-23 23:16:26]

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

[iannucci, 2015-05-23 23:16:27]

+dnj who was interested

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

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:32: // DSNewKeyer allows you to
generate a new *datastore.Key in a couple ways.
how about aliasing datastore.Key with something to completely detach wrapper
interface from GAE datastore package? e.g.

type *datastore.Key DSKey

though you also use datastore.TransationOptions below... I haven't read the
whole CL yet, perhaps it is a stupid suggestion.

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:62: // DSCursor wraps
datastore.Cursor.
I don't see it wrapping it...

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:67: // DSIterator wraps
datastore.Iterator.
how about moving all these interfaces to package wrapper/datastore/datastore.go
and getting rid of DS prefix? It then would be datastore.Iterator etc. Unless
you import then and real datastore both at the same time.

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/doc.go:31: //	   each of which would call
wrapper.Set<service>Fctory with the factory
typo: Fctory

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/doc.go:36: //     which will will panic with an
appropriate error for unimplemented
typo: will will

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/featurebreaker.go:14: // A feature is the Name of
some methog on the fake. So if you had:
typo: methog

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/featurebreaker.go:70: // publically-facing method.
If it returns an error, the fake should return
explicitly from the body of this method?

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/datastore.go:18: // by wrapper.DS(c)
wrapper.GetDS(c)?

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/datastore.go:21: return dsImpl{ctx(ci), ci}
how do you choose when to put (or not to put) & when returning interfaces?

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:19: func UseGI(c
context.Context) context.Context {
GetGI? :)

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/taskqueue.go:17: // by wrapper.UseTQ(c)
GetTQ

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/globalinfo.go:33: Namespace(namespace string)
(context.Context, error)
document

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memcache.go:54: // MCCodec is the interface
represting what you can do with a memcache.Codec
typo: represting

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memcache.go:69: // MCInflateCodecer binds a
Memcache to a memcache.Codec.
MCCodecInflater? Or MCCodecFactory

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/taskqueue.go:20: // TQMultiReadWriter allows you
to add or delete abatch of Tasks from a queue.
typo: abatch

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/wrapper.infra_testing:2: "skip_testing": true
why? also it is a list of GOOS now:

skip_testing: ["darwin", "linux", "windows"]

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

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:32: // DSNewKeyer allows you to
generate a new *datastore.Key in a couple ways.
On 2015/05/24 at 19:01:56, Vadim Sh. wrote:
> how about aliasing datastore.Key with something to completely detach wrapper
interface from GAE datastore package? e.g.
> 
> type *datastore.Key DSKey
> 
> though you also use datastore.TransationOptions below... I haven't read the
whole CL yet, perhaps it is a stupid suggestion.

Ah, yes. So I want to do that :) But it's still broken, because of the way that
the appengine SDK serializes *Key fields in structs (for datastore). Basically,
the field entity MUST be a *"appengine/datastore".Key. Not an interface, not an
alias. This means that the user of wrapper needs to import "appengine/datastore"
anyway so they can put *Key objects in their fields. If we change underlying
implementations, suddenly they'll get errors at runtime.

Key is recursive (parent field), so you actually have to redefine Parent() as
well.

Additionally, since *Key is all full of private fields, there's no way to
convert between *"gae/wrapper".Key and a *"appengine/datastore".Key. The only
way to mint a new key is with a Context object. In "appengine/datastore",
Context.Call is mockable so I can at least have it lie about the 2 RPC calls
that minting a key generates. In the new SDK it's completely opaque.

Basically it just sucks :(

Key should have been defined as:

type Key interface {
  ...
  Parent() Key
  ...
}

And the datastore property loader function would basically see if the field's
interface == Key, and if so, inflate it's own concrete *key type to fulfill the 
interface. Similarly, the saver would need to extract the concrete type by doing
a type assertion, and failing that, reconstructing it's own *key type by walking
the key (e.g. copying data via the Key interface).

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:62: // DSCursor wraps
datastore.Cursor.
On 2015/05/24 at 19:01:56, Vadim Sh. wrote:
> I don't see it wrapping it...

wraps meaning that it serves the function of (and in the case of the gae
implementation it has datastore.Cursor as a concrete type). Since nothing uses
reflect to load Cursors, and Cursor isn't recursively defined, we can just use
it's interface (which is just String() string)

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/datastore.go:67: // DSIterator wraps
datastore.Iterator.
On 2015/05/24 at 19:01:56, Vadim Sh. wrote:
> how about moving all these interfaces to package
wrapper/datastore/datastore.go and getting rid of DS prefix? It then would be
datastore.Iterator etc. Unless you import then and real datastore both at the
same time.

Datastore, TaskQueue and Memcache are all intertwined. I agree that such a split
should probably be done, but it would significantly complicate the
implementation (I'm pretty sure). The other thing that is tricky is that if it
were split, there would be a temptation to mix and match implementations.
Because of the way that they intertangle in terms of implementation
(transactions being the really tricky one), we'd have to expose transaction
support for the TaskQueue at the interface level (e.g. have an AddAction item on
the Datastore interface which can add callbacks to be called back on successful
transaction commit), but that would only work for the in-memory testing version,
not for the real gae implementation (THOUGH MAYBE IT COULD, since taskqueue is
really just calling the AddAction RPC in the background... though it's very
specific to taskqueues, and isn't a general callback).

bleh.

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/doc.go:31: //	   each of which would call
wrapper.Set<service>Fctory with the factory
On 2015/05/24 at 19:01:56, Vadim Sh. wrote:
> typo: Fctory

Done

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/doc.go:36: //     which will will panic with an
appropriate error for unimplemented
On 2015/05/24 at 19:01:56, Vadim Sh. wrote:
> typo: will will

Done

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/featurebreaker.go:14: // A feature is the Name of
some methog on the fake. So if you had:
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> typo: methog

Done

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/featurebreaker.go:70: // publically-facing method.
If it returns an error, the fake should return
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> explicitly from the body of this method?

Added example

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/datastore.go:18: // by wrapper.DS(c)
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> wrapper.GetDS(c)?

ah, oops. Fixed.

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/datastore.go:21: return dsImpl{ctx(ci), ci}
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> how do you choose when to put (or not to put) & when returning interfaces?

Not sure I understand what you mean?

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:19: func UseGI(c
context.Context) context.Context {
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> GetGI? :)

argh

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/taskqueue.go:17: // by wrapper.UseTQ(c)
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> GetTQ

...

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/globalinfo.go:33: Namespace(namespace string)
(context.Context, error)
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> document

Done (as much as we can)

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memcache.go:54: // MCCodec is the interface
represting what you can do with a memcache.Codec
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> typo: represting

me not gud @ ssplings

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/memcache.go:69: // MCInflateCodecer binds a
Memcache to a memcache.Codec.
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> MCCodecInflater? Or MCCodecFactory

MCCodecInflater

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/taskqueue.go:20: // TQMultiReadWriter allows you
to add or delete abatch of Tasks from a queue.
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> typo: abatch

done

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/taskqueue.go:20: // TQMultiReadWriter allows you
to add or delete abatch of Tasks from a queue.
On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> typo: abatch

done

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/wrapper.infra_testing:2: "skip_testing": true
oh,oops... we have tests now. don't want this.

[Vadim Sh., 2015-05-24 20:15:36]

lgtm, but you should probably wait for review from M-A too, since I'm not a very
strict reviewer...

Also can you put some end-to-end example of how to use all these interfaces in
real application? Well, not end-to-end, but the one that starts with "you have
context.Context with all binding already set".

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

https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
go/src/infra/gae/libs/wrapper/gae/datastore.go:21: return dsImpl{ctx(ci), ci}
On 2015/05/24 20:04:31, iannucci wrote:
> On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> > how do you choose when to put (or not to put) & when returning interfaces?
> 
> Not sure I understand what you mean?

I would have written:
return &dsImpl{ctx(ci), ci}

Because value types boxed into an interface look alien to me (since I'm used to
"traditional" ref-based languages).

[iannucci, 2015-05-24 20:19:41]

On 2015/05/24 at 20:15:36, vadimsh wrote:
> lgtm, but you should probably wait for review from M-A too, since I'm not a
very strict reviewer...
> 
> Also can you put some end-to-end example of how to use all these interfaces in
real application? Well, not end-to-end, but the one that starts with "you have
context.Context with all binding already set".
> 
>
https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
> File go/src/infra/gae/libs/wrapper/gae/datastore.go (right):
> 
>
https://codereview.chromium.org/1151473003/diff/60001/go/src/infra/gae/libs/w...
> go/src/infra/gae/libs/wrapper/gae/datastore.go:21: return dsImpl{ctx(ci), ci}
> On 2015/05/24 20:04:31, iannucci wrote:
> > On 2015/05/24 at 19:01:57, Vadim Sh. wrote:
> > > how do you choose when to put (or not to put) & when returning interfaces?
> > 
> > Not sure I understand what you mean?
> 
> I would have written:
> return &dsImpl{ctx(ci), ci}
> 
> Because value types boxed into an interface look alien to me (since I'm used
to "traditional" ref-based languages).

Oh, I think I did that because it used to just contain a single pointer (and
since it's non-mutating, it seemed silly to have a pointer-to-a-pointer for no
reason). I'll make it a pointer type now since it contains 2 (!!) pointers.

[M-A Ruel, 2015-05-25 17:14:52]

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg.go:8: "golang.org/x/net/context"
Why golang.org and github.com not be grouped together. Personally, I only use 2
groups: stdlib and everything else. That's what goimports generates by default
anyway.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/context.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/context.go:10: globalInfoKey = 0
why it's not of type key?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/datastore.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/datastore.go:99: // datastore. The function
recieves a context without the ability to enter a
receives

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/datastore.go:123: if obj == nil || obj.(DSFactory)
== nil {
obj.(DSFactory) == nil will panic(), what you want is:

if f, ok := c.Value(datastoreKey).(DSFactory); ok {
  return f(c)
}
return nil

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy.go:35: switch parts[1][len(parts[1])-2:] {
I highly recommend to check the package name too.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy.go:47: funcName = parts[2]
why no break?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy_test.go:30: So(GetTimeNow(c),
ShouldHappenAfter, now)
FYI, that fails on Windows and likely on oversubscribed VMs.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/featurebreaker.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:27: UnsetBrokenFeatures(feature
...string)
UnbreakFeatures() ?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:35: broken map[string]struct{}
broken map[string]bool

or much better:

broken map[string]error

and remove member err. This mean changing the interface above.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:55: for _, f := range feature {
it'd be good to add a check:
if b.broken == nil {
  b.broken = map[string]error{}
}

this way you don't need NewBrokenFeatures(). The main reason is that you also
export BrokenFeatures, so the odd of making an error and forgetting to
initialize the embedded struct is high.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:56: b.broken[f] = struct{}{}
b.broken[f] = true

is faster.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:95: toks := strings.Split(name,
".")
toks := strings.Split(runtime.FuncForPC(fn).Name(), ".")

otherwise line 96 is confusing because you reuse name for two different
purposes.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:98: // Allow users to override
IsBroken
This should be in docstring then.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:103: if _, ok := b.broken[name];
ok {
I don't like that it's not checking the package. It's easy to get a false
positive.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/featurebreaker_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker_test.go:15: *BrokenFeatures
So this way you could embed the struct, not a pointer.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/gae/datastore.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/datastore.go:67: func (g *dsImpl)
Debugf(format string, args ...interface{}) { g.Goon.Context.Debugf(format,
args...) }
I thought the logger would be a separate struct, that would be exposed here but
by forwarding to the logger implementation associated to this context.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/datastore.go:99: },
(*datastore.TransactionOptions)(opts))
(*datastore.TransactionOptions)(opts) ? Why?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/gae/globalinfo.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:42: func (g giImpl)
DefaultVersionHostname() string {
I think I'd prefer to have the methods in alphabetical order, but whatever.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:67: func (g giImpl) Datacenter()
string                  { return appengine.Datacenter() }
Why some one line and other 3 lines? I'd prefer consistency here. Especially
w.r.t. the other files.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:33: // Namespace takes the new
namespace as a string, and returns a context
I think it's better to leave it out.

If we don't know what's the use case of a function and do not expect to use it,
we should probably leave it out. Adding it back once it's actually needed is
likely less painful.

I don't mind that much, having near 100% coverage is also useful in some ways.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:63: if obj == nil || obj.(GIFactory)
== nil {
http://golang.org/ref/spec#Type_assertions

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/mathrand.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/mathrand.go:13: // MathRandFactory is the function
signature for factory methods compatible with
Why? I mean, I can see why a pseudo-random is useful for deterministically
reproducing a unit test but I don't see why it should be exposed.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/taskqueue_testable.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/taskqueue_testable.go:21: AddQueue(queueName
string)
CreateQueue?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/time.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/time.go:43: f := func(context.Context) time.Time {
return *t }
I'd write the function as:

if t == nil {
  return SetTimeNowFactory(c, nil)
}
return SetTimeNowFactory(c, func(context.Context) time.Time { return *t })

otherwise I think you create a closure even when unnecessary and I think the
above wouldn't (not 100% sure). There's some dynamic compilation involved.

[iannucci, 2015-05-26 18:25:07]

PTAL

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg.go:8: "golang.org/x/net/context"
On 2015/05/25 at 17:14:50, M-A Ruel wrote:
> Why golang.org and github.com not be grouped together. Personally, I only use
2 groups: stdlib and everything else. That's what goimports generates by default
anyway.

Ok, I was unsure of this. I was trying to do something like what we do for
python:

  stdlib

  [appengine imports because they're pseudo-stdlib]

  external (not owned by us) libs

  internal (owned by us)

I was considering golang.org/x to be 'stdlib' for all practical purposes.

Do we want to just do 2 groups, pure-stdlib (like: real stdlib, not
stdlib+appengine), and then everything else?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/context.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/context.go:10: globalInfoKey = 0
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> why it's not of type key?

DERP

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/datastore.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/datastore.go:99: // datastore. The function
recieves a context without the ability to enter a
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> receives

done

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/datastore.go:123: if obj == nil || obj.(DSFactory)
== nil {
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> obj.(DSFactory) == nil will panic(), what you want is:
> 
> if f, ok := c.Value(datastoreKey).(DSFactory); ok {
>   return f(c)
> }
> return nil

I was counting on it panic'ing. If it's not nill, and it's some other type, then
that's a bigtime error. The only thing that can set that key is internal to this
package, and it can only ever set it to a DSFactory.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy.go:35: switch parts[1][len(parts[1])-2:] {
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> I highly recommend to check the package name too.

ni() is internal to this package, so it's always going to be us

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy.go:47: funcName = parts[2]
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> why no break?

b/c there's no loop?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy_test.go:30: So(GetTimeNow(c),
ShouldHappenAfter, now)
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> FYI, that fails on Windows and likely on oversubscribed VMs.

yeah I was wondering.... should I sleep for a second or something? I hate
sleeping in tests :(

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/featurebreaker.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:27: UnsetBrokenFeatures(feature
...string)
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> UnbreakFeatures() ?

And BreakFeatures? sgtm.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:35: broken map[string]struct{}
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> broken map[string]bool
> 
> or much better:
> 
> broken map[string]error
> 
> and remove member err. This mean changing the interface above.

I thought map[string]struct{} was better because sizeof(struct{}) == 0. That
said, I like map[string]error, as it was something I was planning on changing
this to eventually. I just thought of a way to make it useful right now though
(see new interface and commonErrors package).

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:55: for _, f := range feature {
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> it'd be good to add a check:
> if b.broken == nil {
>   b.broken = map[string]error{}
> }
> 
> this way you don't need NewBrokenFeatures(). The main reason is that you also
export BrokenFeatures, so the odd of making an error and forgetting to
initialize the embedded struct is high.

good point, added initLocked().

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:56: b.broken[f] = struct{}{}
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> b.broken[f] = true
> 
> is faster.

did the error map.

Also, do you have benchmarks handy? I was under the impression that struct{}{}
was faster and smaller, though we chatted and you pointed out that the struct{}
might need to be addressable (and thus: allocated), which would definitely be
bad.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:95: toks := strings.Split(name,
".")
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> toks := strings.Split(runtime.FuncForPC(fn).Name(), ".")
> 
> otherwise line 96 is confusing because you reuse name for two different
purposes.

ew gross, good catch.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:98: // Allow users to override
IsBroken
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> This should be in docstring then.

done

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker.go:103: if _, ok := b.broken[name];
ok {
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> I don't like that it's not checking the package. It's easy to get a false
positive.

Discussed in chat, it's not exactly clear what to check the package against, and
I think it's OK to ignore the package because this struct is all about checking
the /interface/, which is agnostic to the package.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/featurebreaker_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker_test.go:15: *BrokenFeatures
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> So this way you could embed the struct, not a pointer.

I won't be able to initialize the map on-demand then, will I?

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/gae/datastore.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/datastore.go:67: func (g *dsImpl)
Debugf(format string, args ...interface{}) { g.Goon.Context.Debugf(format,
args...) }
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> I thought the logger would be a separate struct, that would be exposed here
but by forwarding to the logger implementation associated to this context.

I think this is a leftover before logging had it's own gaelogger concept.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/datastore.go:99: },
(*datastore.TransactionOptions)(opts))
On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> (*datastore.TransactionOptions)(opts) ? Why?

er... good question. I think this is also a leftover when I foolishly thought
that I could rely on my own interface types instead of the datastore packages'.
I could technically have my own type for this, since it's trivial to convert,
but for consistency (for now), I'm going to use the SDK's types. Fixed.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/gae/globalinfo.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:42: func (g giImpl)
DefaultVersionHostname() string {
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> I think I'd prefer to have the methods in alphabetical order, but whatever.

done

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/globalinfo.go:67: func (g giImpl) Datacenter()
string                  { return appengine.Datacenter() }
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> Why some one line and other 3 lines? I'd prefer consistency here. Especially
w.r.t. the other files.

I wanted them to all be 1line, but gofmt apparently knows better. I guess I'll
make them 3 lines each.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:33: // Namespace takes the new
namespace as a string, and returns a context
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> I think it's better to leave it out.
> 
> If we don't know what's the use case of a function and do not expect to use
it, we should probably leave it out. Adding it back once it's actually needed is
likely less painful.
> 
> I don't mind that much, having near 100% coverage is also useful in some ways.

I actually do plan on using it, and I know some other devs are thinking about
using it as well. It's a pretty good way to implement multi-tenancy.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:63: if obj == nil || obj.(GIFactory)
== nil {
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> http://golang.org/ref/spec#Type_assertions

http://play.golang.org/p/g1bN9UNSkS

Typed v. untyped nil w.r.t functions. :)

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/mathrand.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/mathrand.go:13: // MathRandFactory is the function
signature for factory methods compatible with
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> Why? I mean, I can see why a pseudo-random is useful for deterministically
reproducing a unit test but I don't see why it should be exposed.

Exposed to who? This is just an interface. There could be an implementation
which always returns 1, or one that uses a real Rand. That said, its probably
bad that I return a *Rand, but I don't really want to fix that right now.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/taskqueue_testable.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/taskqueue_testable.go:21: AddQueue(queueName
string)
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> CreateQueue?

done

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/time.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/time.go:43: f := func(context.Context) time.Time {
return *t }
On 2015/05/25 at 17:14:52, M-A Ruel wrote:
> I'd write the function as:
> 
> if t == nil {
>   return SetTimeNowFactory(c, nil)
> }
> return SetTimeNowFactory(c, func(context.Context) time.Time { return *t })
> 
> otherwise I think you create a closure even when unnecessary and I think the
above wouldn't (not 100% sure). There's some dynamic compilation involved.

cool done.

[iannucci, 2015-05-26 22:11:02]

FTR; I'm waiting for M-A for final stamp :)

(not being impatient, just making sure we're all on the same page)

[M-A Ruel, 2015-05-27 00:27:12]

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg.go:8: "golang.org/x/net/context"
On 2015/05/26 18:25:05, iannucci wrote:
> On 2015/05/25 at 17:14:50, M-A Ruel wrote:
> > Why http://golang.org and http://github.com not be grouped together.
Personally, I only use
> 2 groups: stdlib and everything else. That's what goimports generates by
default
> anyway.
> 
> Ok, I was unsure of this. I was trying to do something like what we do for
> python:
> 
>   stdlib
> 
>   [appengine imports because they're pseudo-stdlib]
> 
>   external (not owned by us) libs
> 
>   internal (owned by us)
> 
> I was considering golang.org/x to be 'stdlib' for all practical purposes.
> 
> Do we want to just do 2 groups, pure-stdlib (like: real stdlib, not
> stdlib+appengine), and then everything else?

I prefer the result of the following:

pack everything in a single block
run goimports -w .

whatever is generated by that becomes the golden standard.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy.go:47: funcName = parts[2]
On 2015/05/26 18:25:06, iannucci wrote:
> On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> > why no break?
> 
> b/c there's no loop?

That ... is a good reason. I misread as a for loop.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/dummy_test.go:30: So(GetTimeNow(c),
ShouldHappenAfter, now)
On 2015/05/26 18:25:06, iannucci wrote:
> On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> > FYI, that fails on Windows and likely on oversubscribed VMs.
> 
> yeah I was wondering.... should I sleep for a second or something? I hate
> sleeping in tests :(

Use >=, this will catch 99.9999% of the cases except when the clock goes
backward but it's infrequent enough it's not worth optimizing for.

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/featurebreaker_test.go (right):

https://codereview.chromium.org/1151473003/diff/100001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/featurebreaker_test.go:15: *BrokenFeatures
On 2015/05/26 18:25:06, iannucci wrote:
> On 2015/05/25 at 17:14:51, M-A Ruel wrote:
> > So this way you could embed the struct, not a pointer.
> 
> I won't be able to initialize the map on-demand then, will I?

Yes, since you need to grab the lock in each method anyway.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/meta/eg.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/meta/eg.go:35: ds := wrapper.GetDS(c)
you could inline ds at line 40 unless you want to use some form of convention.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/brokenfeatures.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:51: func NewBrokenFeatures(err
error) *BrokenFeatures {
I still think this function is unneeded.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:73: b.initLocked()
Inline initLocked here since it's now the only call site.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:85: b.initLocked()
It's not needed, delete(nil, ...) works, e.g.
http://play.golang.org/p/WI0_F4m_Rz

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:119: b.initLocked()
it's not needed here.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:126: for {
for off := 1; ; off++ {

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:140: off++
remove

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:146: off++
remove

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/context.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/context.go:10: globalInfoKey key
I'd prefer for consistency even if unneeded:
globalInfoKey key = 0

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/datastore.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/datastore.go:122: obj := c.Value(datastoreKey)
if obj := c.Value(datastoreKey); obj != nil && obj.(DSFactory) != nil {
  return obj.(DSFactory)(c)
}
return nil

you save a line. A-M-A-Z-I-N-G.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go:17: ErrCASConflictMC =
memcache.ErrCASConflict
Why the aliasing exactly?

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go:38: StructType:
structType, FieldName: fieldName, Reason: reason}
then one item per line.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:62: obj := c.Value(globalInfoKey)
you could use the "save a line" trick here too.

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/globalinfo.go:69: // SetGIFactory sets the
function to produce GlobalInfo instances, as returned by
I'd prefer to wrap at 80 here

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/mathrand.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/mathrand.go:24: return
rand.New(rand.NewSource(GetTimeNow(c).UnixNano()))
why not return crypto rand?

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/memcache.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/memcache.go:89: obj := c.Value(memcacheKey)
line saveable

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/taskqueue.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/taskqueue.go:62: obj := c.Value(taskQueueKey)
same pattern

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
File go/src/infra/gae/libs/wrapper/testable.go (right):

https://codereview.chromium.org/1151473003/diff/140001/go/src/infra/gae/libs/...
go/src/infra/gae/libs/wrapper/testable.go:8: type Testable interface {
Do you plan to add more stuff? I forget.

[iannucci, 2015-05-27 02:40:57]

ptal, I actually did the save-a-line for everything before, but somehow lost it
(I think rebase?)

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/meta/eg.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/meta/eg.go:35: ds := wrapper.GetDS(c)
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> you could inline ds at line 40 unless you want to use some form of convention.

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/brokenfeatures.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:51: func NewBrokenFeatures(err
error) *BrokenFeatures {
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> I still think this function is unneeded.

Removed, made dfltErr exported

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:73: b.initLocked()
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> Inline initLocked here since it's now the only call site.

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:85: b.initLocked()
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> It's not needed, delete(nil, ...) works, e.g.
> http://play.golang.org/p/WI0_F4m_Rz

cool, done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:119: b.initLocked()
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> it's not needed here.

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:126: for {
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> for off := 1; ; off++ {

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:140: off++
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> remove

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:146: off++
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> remove

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/context.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/context.go:10: globalInfoKey key
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> I'd prefer for consistency even if unneeded:
> globalInfoKey key = 0

golint complains (and thus I fail presubmit) :(

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/datastore.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/datastore.go:122: obj := c.Value(datastoreKey)
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> if obj := c.Value(datastoreKey); obj != nil && obj.(DSFactory) != nil {
>   return obj.(DSFactory)(c)
> }
> return nil
> 
> you save a line. A-M-A-Z-I-N-G.

I THOUGHT I FIXED ALL THESE!!!!

(/me did it on the wrong branch :()

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go:17: ErrCASConflictMC =
memcache.ErrCASConflict
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> Why the aliasing exactly?

So then you can BreakFeatures(commonErrors.Something, "Feature", ...)

I'll be adding more internal errors here once the memory-testing CL lands too.
There are lots :(.

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/gae/commonErrors/errors.go:38: StructType:
structType, FieldName: fieldName, Reason: reason}
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> then one item per line.

done.

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/globalinfo.go:62: obj := c.Value(globalInfoKey)
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> you could use the "save a line" trick here too.

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/globalinfo.go:69: // SetGIFactory sets the
function to produce GlobalInfo instances, as returned by
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> I'd prefer to wrap at 80 here

find/replace fail

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/mathrand.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/mathrand.go:24: return
rand.New(rand.NewSource(GetTimeNow(c).UnixNano()))
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> why not return crypto rand?

Doesn't support the same interface (though crypto rand is much better). I don't
remember the specific reason, but I think something needed math/rand? I'll
probably add a crypto rand too later (that's why this one is called MathRand
anyway)

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

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/memcache.go:89: obj := c.Value(memcacheKey)
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> line saveable

done

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

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/taskqueue.go:62: obj := c.Value(taskQueueKey)
On 2015/05/27 at 00:27:11, M-A Ruel wrote:
> same pattern

done

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/testable.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/140001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/testable.go:8: type Testable interface {
On 2015/05/27 at 00:27:12, M-A Ruel wrote:
> Do you plan to add more stuff? I forget.

yes but not in this cl. If I don't add more stuff, I'll remove this interface in
favor of just FeatureBreaker.

[iannucci, 2015-05-27 02:45:00]

oh, and all the imports are in goimports order already. standardizing on
whatever goimports spits out sgtm.

[M-A Ruel, 2015-05-27 12:31:46]

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/brokenfeatures.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:117: // TODO(riannucci): Profile
this to see if it's having an adverse
I'm concerned that you keep the lock while calling runtime.Caller() but I'm fine
with leaving it as a follow up.
Basically,
- lock, check if b.broken is empty, early exit if appropriate.
- unlock, accumulate the items in call stack in a local array.
- lock check if in b.broken, return.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/brokenfeatures_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:16: *BrokenFeatures
It doesn't need to be a pointer anymore.
Everywhere.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:56: e := fmt.Errorf("sup")
errors.New

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:60:
Convey("BrokenFeatures", t, func() {
Frankly, each could be a small test case and it'd be more readable, as I noted
to Vadim, not a fan of Convey.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:112: err := error(nil)
I often use
var err error
var wg sync.WaitGroup

for uninitialized local variables. I don't mind much.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:117: bf :=
&BrokenFeatures{DefaultError: e}
doesn't need to be a pointer.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:121: err = bf.IsBroken()
With Convey, you can do checks in other goroutines? Bleh.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/dummy_test.go:30: So(GetTimeNow(c),
ShouldHappenAfter, now)
As I said previously, please use ShouldHappenOnOrAfter

[iannucci, 2015-05-27 17:06:55]

ptal again. I think I actually got everything this time.

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/brokenfeatures.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures.go:117: // TODO(riannucci): Profile
this to see if it's having an adverse
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> I'm concerned that you keep the lock while calling runtime.Caller() but I'm
fine with leaving it as a follow up.
> Basically,
> - lock, check if b.broken is empty, early exit if appropriate.
> - unlock, accumulate the items in call stack in a local array.
> - lock check if in b.broken, return.

done

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/brokenfeatures_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:16: *BrokenFeatures
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> It doesn't need to be a pointer anymore.
> Everywhere.

yep done

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:56: e := fmt.Errorf("sup")
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> errors.New

done

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:60:
Convey("BrokenFeatures", t, func() {
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> Frankly, each could be a small test case and it'd be more readable, as I noted
to Vadim, not a fan of Convey.

I actually disagree, and I'm a fan of Convey (now that I've rewritten their core
logic) :)

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:112: err := error(nil)
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> I often use
> var err error
> var wg sync.WaitGroup
> 
> for uninitialized local variables. I don't mind much.

done

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:117: bf :=
&BrokenFeatures{DefaultError: e}
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> doesn't need to be a pointer.

done

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/brokenfeatures_test.go:121: err = bf.IsBroken()
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> With Convey, you can do checks in other goroutines? Bleh.

done, I just didn't do it before

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/160001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/dummy_test.go:30: So(GetTimeNow(c),
ShouldHappenAfter, now)
On 2015/05/27 at 12:31:46, M-A Ruel wrote:
> As I said previously, please use ShouldHappenOnOrAfter

this cl is too big :( done

[M-A Ruel, 2015-05-27 17:14:58]

lgtm

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/dummy_test.go:84: So(GetTimeNow(c),
ShouldHappenAfter, now)
OnOrAfter

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/globalinfo.go:34: // set to use that namespace, or
an error. I'm not sure what the error
I'd remove the "I'm not sure ..."

An error is an error. :)

The moment when it's useful to document this is when the error is typed, e.g.

type FooErr struct {
  ..
}
func (f*FooErr) Error() string {
}

so that clients can try casting it to detect different kind of runtime failure.

[iannucci, 2015-05-27 17:25:06]

The CQ bit was checked by iannucci@chromium.org

[iannucci, 2015-05-27 17:25:07]

fixed. thanks for the review!

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/dummy_test.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/dummy_test.go:84: So(GetTimeNow(c),
ShouldHappenAfter, now)
On 2015/05/27 at 17:14:57, M-A Ruel wrote:
> OnOrAfter

I thought this was testing a mocked clock :( fixed

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
File go/src/infra/gae/libs/wrapper/globalinfo.go (right):

https://chromiumcodereview.appspot.com/1151473003/diff/180001/go/src/infra/ga...
go/src/infra/gae/libs/wrapper/globalinfo.go:34: // set to use that namespace, or
an error. I'm not sure what the error
On 2015/05/27 at 17:14:57, M-A Ruel wrote:
> I'd remove the "I'm not sure ..."
> 
> An error is an error. :)
> 
> The moment when it's useful to document this is when the error is typed, e.g.
> 
> type FooErr struct {
>   ..
> }
> func (f*FooErr) Error() string {
> }
> 
> so that clients can try casting it to detect different kind of runtime
failure.

I clarified the comment.

[iannucci, 2015-05-27 17:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
vadimsh@chromium.org, maruel@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1151473003/#ps210030 (title: "fix
coverage numbers")

[commit-bot: I haz the power, 2015-05-27 17:25:37]

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

[commit-bot: I haz the power, 2015-05-27 17:28:59]

Committed patchset #13 (id:210030) as
https://chromium.googlesource.com/infra/infra/+/f5450c53963f2883920e83ab2564b...